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 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.


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. 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. 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.


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.

Bye...


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