On Sun, 2010-11-07 at 09:31 +0100, Gilles Chanteperdrix wrote:
> Jan Kiszka wrote:
> >>> Anyway, after some thoughts, I think we are going to try and make the
> >>> current situation work instead of going back to the old way.
> >>>
> >>> You can find the patch which attempts to do so here:
> >>> http://sisyphus.hd.free.fr/~gilles/sched_status.txt
> >> Ack. At last, this addresses the real issues without asking for
> >> regression funkiness: fix the lack of barrier before testing XNSCHED in
> > 
> > Check the kernel, we actually need it on both sides. Wherever the final
> > barriers will be, we should leave a comment behind why they are there.
> > Could be picked up from kernel/smp.c.
> 
> We have it on both sides: the non-local flags are modified while holding
> the nklock. Unlocking the nklock implies a barrier.

I think we may have an issue with this kind of construct:

xnlock_get_irq*(&nklock)
        xnpod_resume/suspend/whatever_thread()
                xnlock_get_irq*(&nklock)
                ...
                xnlock_put_irq*(&nklock)
        xnpod_schedule()
                xnlock_get_irq*(&nklock)
                        send_ipi
                        =====> xnpod_schedule_handler on dest CPU
                xnlock_put_irq*(&nklock)
xnlock_put_irq*(&nklock)

The issue would be triggered by the use of recursive locking. In that
case, the source CPU would only sync its cache when the lock is actually
dropped by the outer xnlock_put_irq* call and the inner
xnlock_get/put_irq* would not act as barriers, so the remote
rescheduling handler won't always see the XNSCHED update done remotely,
and may lead to a no-op. So we need a barrier before sending the IPI in
__xnpod_test_resched().

This could not happen if all schedule state changes where clearly
isolated from rescheduling calls in different critical sections, but
it's sometimes not an option not to group them for consistency reasons.

> 
> > 
> >> the xnpod_schedule pre-test, and stop sched->status trashing due to
> >> XNINIRQ/XNHTICK/XNRPICK ops done un-synced on nklock.
> >>
> >> In short, this patch looks like moving the local-only flags where they
> >> belong, i.e. anywhere you want but *outside* of the status with remotely
> >> accessed bits. XNRPICK seems to be handled differently, but it makes
> >> sense to group it with other RPI data as you did, so fine with me.
> > 
> > I just hope we finally converge over a solution. Looks like all
> > possibilities have been explored now. A few more comments on this one:
> > 
> > It probably makes sense to group the status bits accordingly (both their
> > values and definitions) and briefly document on which status field they
> > are supposed to be applied.
> 
> Ok, but I wanted them to not use the same values, so that we can use the
> sched->status | sched->lstatus trick in xnpod_schedule. Something is
> lacking too: we probably need to use sched->status | sched->lstatus for
> display in /proc.
> 
> > 
> > I do not understand the split logic - or some bits are simply not yet
> > migrated. XNHDEFER, XNSWLOCK, XNKCOUT are all local-only as well, no?
> > Then better put them in the _local_ status field, that's more consistent
> > (and would help if we once wanted to optimize their cache line usage).
> 
> Maybe the naming is not good the. ->status is everything which is
> modified under nklock, ->lstatus is for XNINIRQ and XNHTICK which are
> modified without holding the nklock.
> 
> > 
> > The naming is unfortunate: status vs. lstatus. This is asking for
> > confusion and typos. They must be better distinguishable, e.g.
> > local_status. Or we need accessors that have debug checks built in,
> > catching wrong bits for their target fields.
> 
> I agree.
> 
> > 
> > Good catch of the RPI breakage, Gilles!
> 

-- 
Philippe.



_______________________________________________
Xenomai-core mailing list
Xenomai-core@gna.org
https://mail.gna.org/listinfo/xenomai-core

Reply via email to