This is the mail archive of the ecos-patches@sourceware.org mailing list for the eCos project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: FIFO DSRs... the 2nd


Hi Stefan,

"Stefan Sommerfeld" <sommerfeld@mikrom.de> writes:
> Hi Sergei,
>>>>>
>>>>> I don't think your implementation of call_pending_DSRs_inner() is
>>>>> save. You cannot reset the dsr_list and dsr_list_tail entries before
>>>>> running the dsr routines, because the irqs are activated while the dsr
>>>>> routines run, so another irq with a dsr could happend. This will
>>>>> result in a change of the dsr order and also dsr could be missed
>>>>> because the irq->next pointer is changed. The dsr_list and
>>>>> dsr_list_tail pointer must be current while the dsrs are running.
>>>>
>>>> I believe you are wrong here.
>>>>
>>>> Under interrupts disabled I take the entire current list of DSRs out of
>>>> sight of post_dsr() routine (except it can modify 'dsr_count' of an ISR
>>>> in the list), so if post_dsr() is called while I handle current
>>>> DSRs, it
>>>> will add DSR to a fresh list that will later be handled at top
>>>> level (in
>>>> the unlock_inner()).
>>>
>>> No, i think you're wrong. You save the pointer to the first dsr, but
>>> not the list. The irq structures are the elements and the next pointer
>>> can be modified.
>>>
>>> Example:
>>>
>>> You have 3 dsr's pending on the call_pending_DSRs_inner() call (let's
>>> call the dsr a,b and c). You save a pointer to dsr a for you dsr
>>> calling loop. The dsr routine will of dsr a will be called and before
>>> it returns another irq happened. This is the irq for dsr b. The
>>> dsr_list will be set to dsr b and and next pointer of dsr b will be
>>> set to 0 because the dsr_list_tail pointer was 0.
>>
>> No. If DSR "b" is already in the list I'm handling, its 'dsr_count'
>> field is non-zero. Should ISR "b" fire, the post_dsr() will simply
>> increment the "b->dsr_count". It won't change "b->next_dsr".
>
> But the IRQ doesn't know the dsr b is already on the list because you
> cleared the dsr_list and dsr_list_tail pointers.

It does know because "dsr_count" is non-zero. It doesn't care about
pointers if "dsr_count" is non-zero on entry.

Please look at the post_dsr() -- it only checks for "dsr_count" to be
non-zero, it begins with:

    if( dsr_count++ == 0 )
    {

and does nothing else when the 'if' is not taken. Where do you see the
dsr_list and dsr_list_tail are checked?!

>
>> Please notice that the above is not different from your
>> implementation. Every ISR has only one "next_dsr" field, so it can't be
>> put twice into the list, -- that's why "dsr_count" is there in the first
>> place, -- to prevent the scenario you've just described. I.e., if what
>> you describe were indeed the case, neither LIFO nor your algorithm
>> modelled after LIFO one would work correctly.
>
> No, my implementation only removes the head object from the dsr_list
> and processes it. The list is still intact. If another irq happened,
> the new dsr will just be enqueued at the tail of the dsr_list.

No, it will not, provided it's already in the list. Its 'dsr_count' will
be incremented instead, -- you just can't have an ISR enqueued two times
into the list as it has single 'next_dsr' field. It will do exactly the
same thing in my case.

If it's not in the list, then it will be enqueued to the FIFO both in
your case and in my case. The *only* difference is that in my case it
will be enqueued to the empty FIFO. Moreover, it could happen (the
addition to the empty FIFO) in your case as well during the last pass
through the loop.

> Your version clears the dsr_list and dsr_list_tail pointer, so a new
> IRQ would "think" there's no dsr in the queue and just use the
> ->next_dsr pointer.

No, see above. The post_dsr() routine only changes 'dsr_count' field
provided the field is already non-zero on entry.

> I could make a more detailed example of the problem if you like.
>>
>>> You continue to call the dsr routines and after because dsr b don't
>>> have the next pointer to dsr c anymore, dsr c will never be
>>> called. This will not happen with my implementation. Please go back to
>>> my implementation, because it's save and well tested.
>>
>> Well, I think mine is also well tested. I tried both and both work for
>> me. Did you actually try my patch and it failed, or are you just
>> guessing?
>>
>> BTW, it would be very interesting, at least for me, if your patch work
>> and mine doesn't, because my patch stress-tests the code in
>> call_pending_DSRs_inner() and maybe can shed some light on the "missing
>> DSR" problem(s) others have encountered.
>
> I don't have the time to setup a stress test. I need a stable system
> and i had a lot of trouble in the past with missing dsr's. So if i see
> a problem by just looking at the code, i don't need to test it.

Sorry, I still believe there is no problem.

> I agree with you with the fifo variant and the default setting,
> because the lifo behaviour has to much bad side effects, but the
> implementation must be 100% save.

Sure, and I still believe it is safe.

-- Sergei.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]