Re: [RFC][PATCH 2/9] deadlock prevention core

[Daniel Phillips <phillips@google.com>]

Indan Zupancic wrote:
> Hello,
> Saw the patch on lkml, and wondered about some things.
> On Tue, August 8, 2006 21:33, Peter Zijlstra said:
>
> > +static inline void dev_unreserve_skb(struct net_device *dev)
> > +{
> > +	if (atomic_dec_return(&dev->rx_reserve_used) < 0)
> > +		atomic_inc(&dev->rx_reserve_used);
> > +}
> > +
>
> This looks strange. Is it possible for rx_reserve_used to become negative?
> A quick look at the code says no, except in the one case where there isn't a
> "if (unlikely(dev_reserve_used(skb->dev)))" check:

Yes, you can see what I'm trying to do there, I was short an atomic op to
do it,  My ugly solution may well be... probably is racy.  Let's rewrite
it with something better.  We want the atomic op that some people call
"monus": decrement unless zero.

> > @@ -389,6 +480,8 @@ void __kfree_skb(struct sk_buff *skb)
> > #endif
> >
> > 	kfree_skbmem(skb);
> > +	if (dev && (dev->flags & IFF_MEMALLOC))
> > +		dev_unreserve_skb(dev);
> > }
>
> So it seems that that < 0 check in dev_unreserve_skb() was only added to handle
> this case (though there seems to be a race between those two atomic ops).
> Isn't it better to remove that check and just do:
>
> if (dev && (dev->flags & IFF_MEMALLOC) && dev_reserve_used(dev))

Seems to me that unreserve is also called from each protocol handler.
Agreed, the < 0 check is fairly sickening.

> The use of atomic seems a bit dubious. Either it's necessary, in which case
> changes depending on tests seem unsafe as they're not atomic and something
> crucial could change between the read and the check, or normal reads and writes
> combined with barriers would be sufficient. All in all it seems better to
> move that "if (unlikely(dev_reserve_used(skb->dev)))" check into
> dev_unreserve_skb(), and make the whole atomic if necessary. Then let
> dev_unreserve_skb() return wether rx_reserve_used was positive.
> Getting rid of dev_reserve_used() and using the atomic_read directly might be
> better, as it is set with the bare atomic instructions too and rarely used without
> dev_unreserve_skb().

Barriers should work for this reserve accounting, but is that better than
an atomic op?  I don't know, let's let the barrier mavens opine.

IMHO the cleanest thing to do is code up "monus", in fact I dimly recall
somebody already added something similar.

Side note: please don't be shy, just reply-all in future so the discussion
stays public.  Part of what we do is try to share our development process
so people see not only what we have done, but why we did it.  (And sometimes
so they can see what dumb mistakes we make, but I won't get into that...)

I beg for forgiveness in advance having taken the liberty of CCing this
reply to lkml.

Regards,

Daniel
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/