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

[Peter Zijlstra <a.p.zijlstra@chello.nl>]

On Wed, 2006-08-09 at 20:34 +0200, Indan Zupancic wrote:
> 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.

I know what you ment, and if you've looked at -v2 you'll see that I've
done this, basically because its easier. However the thought behind the
other semantics is, any skb freed will reduce memory pressure.

> Why is it needed for the protocol specific code to call dev_unreserve_skb?

It uses this to get an indication of memory pressure; if we have
memalloc'ed skbs memory pressure must be high, hence we must drop all
non critical packets. But you are right in that this is a problematic
area; the mapping from skb to device is non trivial.

Your suggestion of testing skb->memalloc might work just as good; indeed
if we have regressed into the fallback allocator we know we have
pressure.

> 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?)

Well yes, this is a problem, only today have I understood how volatile
the mapping actually is. I think you are right in that transferring the
accounting from the old to the new device is correct solution.

However this brings us the problem of limiting the fallback allocator;
currently this is done in __netdev_alloc_skb where rx_reserve_used it
compared against rx_reserve. If we transfer accounting away this will
not work anymore. I'll have to think about this case, perhaps we already
have a problem here.

> >> 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.

Very good point indeed.

> >> 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 came up with yes, if for each socket you gain a request queue, the
number of in-flight pages is proportional to the number of sockets.