Re: Multicast packet loss

[David Miller <davem@davemloft.net>]

From: Eric Dumazet <dada1@cosmosbay.com>
Date: Mon, 09 Mar 2009 07:36:57 +0100

> I chose cmpxchg() because I needed some form of exclusion here.
> I first added a spinlock inside "struct softirq_del" then I realize
> I could use cmpxchg() instead and keep the structure small. As the
> synchronization is only needed at queueing time, we could pass
> the address of a spinlock XXX to sofirq_del() call.

I don't understand why you need the mutual exclusion in the
first place.  The function pointer always has the same value.
And this locking isn't protecting the list insertion either,
as that isn't even necessary.

It just looks like plain overhead to me.

> Also, when an event was queued for later invocation, I also needed to keep
> a reference on "struct socket" to make sure it doesnt disappear before
> the invocation. Not all sockets are RCU guarded (we added RCU only for 
> some protocols (TCP, UDP ...). So I found keeping a read_lock
> on callback was the easyest thing to do. I now realize we might
> overflow preempt_count, so special care is needed.

You're using this in UDP so... make the rule that you can't use
this with a non-RCU-quiescent protocol.

> About your first point, maybe we should make sofirq_del() (poor name
> I confess) only have one argument (pointer to struct softirq_del),
> and initialize the function pointer at socket init time. That would
> insure "struct softirq_del" is associated to one callback
> only. cmpxchg() test would have to be done on "next" field then (or
> use the spinlock XXX)

Why?  You run this from softirq safe context, nothing can run other
softirqs on this cpu and corrupt the list, therefore.