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:
@@ -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))
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().