[Click] NotifierQueue on multi-threaded Click problem.

[Eddie Kohler]

Jason,

Other notes on your patch:

- As mentioned, I would rather not hold Master's _task_lock for the duration
of processing the pending list, and the current code tries to fix this
another way.
But let me know what you think.

- On the locks added to NotifierQueue and FullNoteQueue: As you probably
saw, we
already had code in these elements to handle the SMP case.  The code was
commented
out because in tests we saw that locks dropped performance a LOT; I
forget how much,
but it was a very large degradation.  It is not OK to add gratuitous
locks to these
functions.  (If you can prove me wrong, I'd be happy to know.)

However, I think there is a way to solve the problem without locks:
change this pattern

     if (++_sleepiness == SLEEPINESS_TRIGGER)
         _empty_note.sleep();

to this pattern, which re-wakes the relevant Notifier on detecting the
race condition:

     if (++_sleepiness == SLEEPINESS_TRIGGER) {
         _empty_note.sleep();
         if (_head != _tail && !_empty_note.active())
             _empty_note.wake();
     }

This change is checked in.  Does it work for you?

- More on those locks: The Click Queues, Simple/Notifier/FullNote, are
all designed
to be used when there is *at most one* concurrent pusher and *at most
one* concurrent
puller.  They allow pushes and pulls to happen in parallel, but they do
*not* allow
multiple pushes to happen in parallel, or multiple pulls.  This is now
documented.
If you want multiple concurrent pushes, look at MSQueue.

- "The spinlock_bh needed because of push and pull can be called
concurrently from softirq." : What do you mean?  I may be being stupid,
but I don't think ANY pushes and pulls should be called from softirq
context.  What softirq context are you thinking of?  Click runs almost
entirely in user context (i.e. the kernel threads), or at least it should.

- SpinlockBH: I'm pretty sure we will need more than
SpinlockBH to support preemption...

- SpinlockIRQ: Thanks!  Checked in.

Thanks for this patch!! Sorry for the delay.

Eddie


Jason Park (Joonwoo Park) wrote:
> Hi.
> Recently I tried multi-threaded Click to get more performance.
> But I got stopped queue in a few seconds.
> For the test, I connected two click installed machine with e1000 gigabit
> directly as sender and replier.
> The replier made problem on multi-threaded click that run_task() of
> ToDevice() won't be called any more.
> 
> I worked patch for the problem and it contains.
> - FullNoteQueue, NotifierQueue
> 　The spinlock_bh needed because of push and pull can be called
> concurrently from softirq.
>   But I didn't work for SimpleQueue, it might lock needed too.
> - Task
>   I think that the member _pending should be atomic, in more race
> conditional situation, it can be problem. (You can try SLEEPNIESS_TRIGGER=1)
> - Etc.
>   Replace member function of SpinlockIRQ with the things from linux to
> disable preempt when irq disabled. (But I didn't set CONFIG_PREEMPT for
> this test) 
>   Add class SpinlockBH.
>   Fix Master's lock. (Eddie, Am I right?)
>   I'm running linux 2.6.19.2 SMP.
> 
> I'll be pleased if this patch works.
> 
> Jason Park (Joonwoo Park).