On Wed, August 9, 2006 16:00, Peter Zijlstra said: > On Wed, 2006-08-09 at 15:48 +0200, Indan Zupancic wrote: >> On Wed, August 9, 2006 14:54, Peter Zijlstra said: >> > On Wed, 2006-08-09 at 14:02 +0200, Indan Zupancic wrote: >> >> That avoids lots of checks and should guarantee that the >> >> accounting is correct, except in the case when the IFF_MEMALLOC flag is >> >> cleared and the counter is set to zero manually. Can't that be avoided and >> >> just let it decrease to zero naturally? >> > >> > That would put the atomic op on the free path unconditionally, I think >> > davem gets nightmares from that. >> >> I confused SOCK_MEMALLOC with sk_buff::memalloc, sorry. What I meant was >> to unconditionally decrement the reserved usage only when memalloc is true >> on the free path. That way all skbs that increased the reserve also decrease >> it, and the counter should never go below zero. > > OK, so far so good, except we loose the notion of getting memory back > from regular skbs.
I don't understand this, regular skbs don't have anything to do with rx_reserve_used as far as I can see. I'm only talking about keeping that field up to date and correct. rx_reserve_used is only increased by a skb when memalloc is set to true on that skb, so only if that field is set rx_reserve_used needs to be reduced when the skb is freed. Why is it needed for the protocol specific code to call dev_unreserve_skb? Only problem is if the device can change. rx_reserve_used should probably be updated when that happens, as a skb can't use reserved memory on a device it was moved away from. (right?) >> Also as far as I can see it should be possible to replace all atomic >> "if (unlikely(dev_reserve_used(skb->dev)))" checks witha check if >> memalloc is set. That should make davem happy, as there aren't any >> atomic instructions left in hot paths. > > dev_reserve_used() uses atomic_read() which isn't actually a LOCK'ed > instruction, so that should not matter. Perhaps, but the main reason to check memalloc instead of using dev_reserve_used is because the latter doesn't tell which skb did the reservation. >> If IFF_MEMALLOC is set new skbs set memalloc and increase the reserve. > > Not quite, if IFF_MEMALLOC is set new skbs _could_ get memalloc set. We > only fall back to alloc_pages() if the regular path fails to alloc. If the > skb is backed by a page (as opposed to kmem_cache fluff) sk_buff::memalloc > is set. Yes, true. But doesn't matter for the rx_reserve_used accounting, as long as memalloc set means that it did increase rx_reserve_used. > Also, I've been thinking (more pain), should I not up the reserve for > each SOCK_MEMALLOC socket. Up rx_reserve_used or the total ammount of reserved memory? Probably 'no' for both though, as it's either device specific or skb dependent. I'm slowly getting a clearer image of the big picture, I'll take another look when you post the updated code. Greetings, Indan - To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html