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.
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
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.
Currently atomic_inc_not_zero(), atomic_add_unless() and atomic_cmpxchg()
exist, so making an atomic_dec_not_zero() should be easy.
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.
Yes, but those all do that (racy) "if (unlikely(dev_reserve_used(skb->dev)))"
check. Adding another racy check doesn't improve the situation.
quoted
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.
The atomic ops are fine, but if you do two atomic ops then that as a whole
isn't atomic any more and often racy. That's what seems to be the case
here.
IMHO the cleanest thing to do is code up "monus", in fact I dimly recall
somebody already added something similar.
I'm not sure, to me it looks like dev_unreserve_skb() is always called
without really knowing if it is justified or not, or else there wouldn't
be a chance that the counter became negative. So avoiding the negative
reserve usage seems like papering over bad accounting.
The assumption made seems to be that if there's reserve used, then it must
be us using it, and it's unreserved. So it appears that either that
assumption is wrong, and we can unreserve for others while we never
reserved for ourselves, or it is correct, in which case it probably makes
more sense to check for the IFF_MEMALLOC flag.
All in all it seems like a per skb flag which tells us if this skb was the
one reserving anything is missing. Or rx_reserve_used must be updated for
all in flight skbs whenever the IFF_MEMALLOC flag changes, so that we can
be sure that the accounting works correctly. Oh wait, isn't that what the
memalloc flag is for? So shouldn't it be sufficient to check only with
sk_is_memalloc()? 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? So checking IFF_MEMALLOC for new
skbs and use sk_is_memalloc() for existing ones seems workable, if I'm not
missing anything (I think I do).
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...)
Yes, I know, but as I don't have much kernel programming experience I
didn't want to add unnecessary noise.
I beg for forgiveness in advance having taken the liberty of CCing this
reply to lkml.
From: Peter Zijlstra <hidden> Date: 2006-08-09 12:58:30
On Wed, 2006-08-09 at 14:02 +0200, Indan Zupancic wrote:
On Wed, August 9, 2006 2:25, Daniel Phillips said:
quoted
.... We want the atomic op that some people call
"monus": decrement unless zero.
Currently atomic_inc_not_zero(), atomic_add_unless() and atomic_cmpxchg()
exist, so making an atomic_dec_not_zero() should be easy.
atomic_add_unless() - will nicely do, thanks.
I'm not sure, to me it looks like dev_unreserve_skb() is always called
without really knowing if it is justified or not, or else there wouldn't
be a chance that the counter became negative. So avoiding the negative
reserve usage seems like papering over bad accounting.
It was indeed called too often it seems, once when deciding to drop the
skb
and again then actually freeing the skb.
The assumption made seems to be that if there's reserve used, then it must
be us using it, and it's unreserved. So it appears that either that
assumption is wrong, and we can unreserve for others while we never
reserved for ourselves, or it is correct, in which case it probably makes
more sense to check for the IFF_MEMALLOC flag.
Changed it to only dec_not_zero on free for IFF_MEMALLOC devices.
I'm thinking of making kfree_skbmem -> skb_release_data return whether
they
released the actual data and also depend on that.
All in all it seems like a per skb flag which tells us if this skb was the
one reserving anything is missing.
struct sk_buff::memalloc
However the idea is that freeing non memalloc skbs also returns memory
(albeit
to the slab and not the free page list).
Or rx_reserve_used must be updated for
all in flight skbs whenever the IFF_MEMALLOC flag changes, so that we can
be sure that the accounting works correctly.
Yes
Oh wait, isn't that what the
memalloc flag is for? So shouldn't it be sufficient to check only with
sk_is_memalloc()?
See previous comment.
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.
Thanks
On Wed, August 9, 2006 14:54, Peter Zijlstra said:
On Wed, 2006-08-09 at 14:02 +0200, Indan Zupancic wrote:
quoted
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.
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.
If IFF_MEMALLOC is set new skbs set memalloc and increase the reserve.
When the skb is being destroyed it doesn't matter if IFF_MEMALLOC is set
or not, only if that skb used reserves and thus only the memalloc flag
needs to be checked. This means that changing the IFF_MEMALLOC doesn't
affect in-flight skbs but only newly created ones, and there's no need to
update in-flight skbs whenever the flag is changed as all should go well.
+int sk_set_memalloc(struct sock *sk)
+{
+ struct inet_sock *inet = inet_sk(sk);
+ struct net_device *dev = ip_dev_find(inet->rcv_saddr);
+ int err = 0;
+
+ if (!dev)
+ return -ENODEV;
+
+ if (!(dev->features & NETIF_F_MEMALLOC)) {
+ err = -EPERM;
+ goto out;
+ }
+
+ if (atomic_read(&dev->memalloc_socks) == 0) {
+ spin_lock(&dev->memalloc_lock);
+ if (atomic_read(&dev->memalloc_socks) == 0) {
+ dev->memalloc_reserve =
+ dev->rx_reserve * skb_pages(dev->mtu);
+ err = adjust_memalloc_reserve(dev->memalloc_reserve);
+ if (err) {
+ spin_unlock(&dev->memalloc_lock);
+ printk(KERN_WARNING
+ "%s: Unable to allocate RX reserve, error: %d\n",
+ dev->name, err);
+ goto out;
+ }
+ sock_set_flag(sk, SOCK_MEMALLOC);
+ dev->flags |= IFF_MEMALLOC;
+ }
+ atomic_inc(&dev->memalloc_socks);
+ spin_unlock(&dev->memalloc_lock);
+ } else
+ atomic_inc(&dev->memalloc_socks);
+
+out:
+ dev_put(dev);
+ return err;
+}
It seems that here SOCK_MEMALLOC is only set on the first socket.
Shouldn't it be set on all sockets instead?
Greetings,
Indan
From: Peter Zijlstra <hidden> Date: 2006-08-09 14:04:57
On Wed, 2006-08-09 at 15:48 +0200, Indan Zupancic wrote:
On Wed, August 9, 2006 14:54, Peter Zijlstra said:
quoted
On Wed, 2006-08-09 at 14:02 +0200, Indan Zupancic wrote:
quoted
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.
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.
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.
When the skb is being destroyed it doesn't matter if IFF_MEMALLOC is set
or not, only if that skb used reserves and thus only the memalloc flag
needs to be checked. This means that changing the IFF_MEMALLOC doesn't
affect in-flight skbs but only newly created ones, and there's no need to
update in-flight skbs whenever the flag is changed as all should go well.
Your reasoning is sound, except for these two point above...
<snip code>
It seems that here SOCK_MEMALLOC is only set on the first socket.
Shouldn't it be set on all sockets instead?
Ouch, where was my brain... Thanks!
Also, I've been thinking (more pain), should I not up the reserve for
each
SOCK_MEMALLOC socket.
On Wed, August 9, 2006 16:00, Peter Zijlstra said:
On Wed, 2006-08-09 at 15:48 +0200, Indan Zupancic wrote:
quoted
On Wed, August 9, 2006 14:54, Peter Zijlstra said:
quoted
On Wed, 2006-08-09 at 14:02 +0200, Indan Zupancic wrote:
quoted
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?)
quoted
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.
quoted
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
From: Peter Zijlstra <hidden> Date: 2006-08-09 20:01:38
On Wed, 2006-08-09 at 20:34 +0200, Indan Zupancic wrote:
On Wed, August 9, 2006 16:00, Peter Zijlstra said:
quoted
On Wed, 2006-08-09 at 15:48 +0200, Indan Zupancic wrote:
quoted
On Wed, August 9, 2006 14:54, Peter Zijlstra said:
quoted
On Wed, 2006-08-09 at 14:02 +0200, Indan Zupancic wrote:
quoted
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.
quoted
quoted
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.
quoted
quoted
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.
quoted
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.
From: Peter Zijlstra <hidden> Date: 2006-08-09 20:19:56
On Wed, 2006-08-09 at 21:45 +0200, Peter Zijlstra wrote:
On Wed, 2006-08-09 at 20:34 +0200, Indan Zupancic wrote:
quoted
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.
quoted
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.
Humm, if we do not use dev_reserve_used in the protocols anymore, the
only place that still uses it is the fallback limit. If we could solve
that in another way we might be able to get rid of it all together. That
would save the pain of managing the accounting transferal on skb::dev
assignments too.
Daniel, any ideas? I'm fighting to stay awake... ;-)
On Wed, August 9, 2006 21:45, Peter Zijlstra said:
On Wed, 2006-08-09 at 20:34 +0200, Indan Zupancic wrote:
quoted
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.
You seem to have explained dev_reserve_used usage, not the dev_unreserve_skb calls.
But I've just found -v2 and see that they're gone now, great. -v2 looks much better.
quoted
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.
The point of the reservations is to avoid deadlocks, and they're always big
enough to hold all in-flight skbs, right? So what about solving the whole
device problem by using a global counter and limit instead of per device?
The question is whether traffic on one device can starve traffic on other
devices or not, and how big a problem that is. It probably can, tricky stuff.
Though getting rid of the per device stuff would simplify a lot...
quoted
quoted
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.