From: Cong Wang <hidden> Date: 2017-01-12 05:02:27
alloc_tx() is already inside a wait loop for a successful skb
allocation, this loop inside alloc_tx() is quite unnecessary
and actually problematic.
Signed-off-by: Cong Wang <redacted>
---
net/atm/common.c | 9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)
From: Cong Wang <hidden> Date: 2017-01-12 05:02:28
Andrey reported a kernel warning for the blocking ops
in between prepare_to_wait() and schedule(), that is
alloc_tx().
Of course, the logic itself is suspicious, other sendmsg()
could handle skb allocation failure very well, not sure
why ATM has to wait for a successful one here. But probably
it is too late to change since the errno and behavior is
visible to user-space. So just leave the logic as it is.
Reported-by: Andrey Konovalov <redacted>
Tested-by: Andrey Konovalov <redacted>
Signed-off-by: Cong Wang <redacted>
---
net/atm/common.c | 9 ++++-----
1 file changed, 4 insertions(+), 5 deletions(-)
From: Michal Hocko <mhocko@kernel.org> Date: 2017-01-12 08:43:28
On Wed 11-01-17 21:02:01, Cong Wang wrote:
alloc_tx() is already inside a wait loop for a successful skb
allocation, this loop inside alloc_tx() is quite unnecessary
and actually problematic.
I am not familiar with this code at all but vcc_sendmsg seems to be one
of those cases where open coding __GFP_NOFAIL semantic makes sense as
there is an allocation fallback strategy implemented.
Signed-off-by: Cong Wang <redacted>
I cannot give my reviewed-by because I am not familiar with the code but
this looks like an improvement to me.
Were alloc_skb moved one level up in the call stack, there would be
no need to use the new wait api in the subsequent page, thus easing
pre 3.19 longterm kernel maintenance (at least those on korg page).
But it tastes a tad bit too masochistic.
--
Ueimor
Were alloc_skb moved one level up in the call stack, there would be
no need to use the new wait api in the subsequent page, thus easing
pre 3.19 longterm kernel maintenance (at least those on korg page).
alloc_skb(GFP_KERNEL) itself is sleeping, so the new wait api is still
needed.
alloc_skb(GFP_KERNEL) itself is sleeping, so the new wait api is still
needed.
The task state change warning is the symptom.
The deeply nested alloc_skb is the problem.
Diagnosis: nesting is wrong. It makes zero sense. Fix it and the
implicit task state change problem automagically goes away.
alloc_skb() does not need to be in the "while" loop.
alloc_skb() does not need to be in the {prepare_to_wait/add_wait_queue ...
finish_wait/remove_wait_queue} block.
alloc_tx() is not correctly named: given its original content, it deserves
to be called something like:
"wait_for_decent_tx_drain_and_alloc_by_hand_coz_i_dont_trust_the_mm_subsystem_and_i_dont_know_what_i_want"
I claim that:
- alloc_tx() should only perform the "wait_for_decent_tx_drain" part
- alloc_skb() ought to be done directly in vcc_sendmsg
- alloc_skb() failure can be handled gracefully in vcc_sendmsg
- alloc_skb() may use a (m->msg_flags & MSG_DONTWAIT) dependant
GFP_{KERNEL / ATOMIC} flag
- most of it can be done in a longterm maintenance pain minimizing
way. Call it a side-effect: I don't claim that it *must* be done
this way.
--
Ueimor
Were alloc_skb moved one level up in the call stack, there would be
no need to use the new wait api in the subsequent page, thus easing
pre 3.19 longterm kernel maintenance (at least those on korg page).
But it tastes a tad bit too masochistic.
Lack of error handling of allocation failure is always a huge red
flag. We even long ago tried to do something like this for TCP FIN
handling.
It's dumb, it doesn't work.
Therefore I agree that the correct fix is to move the SKB allocation
up one level to vcc_sendmsg() and make it handle errors properly.
From: Cong Wang <hidden> Date: 2017-01-13 18:18:53
On Fri, Jan 13, 2017 at 5:23 AM, Francois Romieu [off-list ref] wrote:
Cong Wang [off-list ref] :
[...]
quoted
alloc_skb(GFP_KERNEL) itself is sleeping, so the new wait api is still
needed.
The task state change warning is the symptom.
The deeply nested alloc_skb is the problem.
Diagnosis: nesting is wrong. It makes zero sense. Fix it and the
implicit task state change problem automagically goes away.
alloc_skb() does not need to be in the "while" loop.
This is exactly what I describe in my changelog, don't know
why you want to repeat it...
alloc_skb() does not need to be in the {prepare_to_wait/add_wait_queue ...
finish_wait/remove_wait_queue} block.
If you ever read the followup patch of this one, you will find:
"
Of course, the logic itself is suspicious, other sendmsg()
could handle skb allocation failure very well, not sure
why ATM has to wait for a successful one here. But probably
it is too late to change since the errno and behavior is
visible to user-space. So just leave the logic as it is.
"
alloc_tx() is not correctly named: given its original content, it deserves
to be called something like:
Please don't expect me to fix many things in one patch, let's
fix each of them separately, agreed?
"wait_for_decent_tx_drain_and_alloc_by_hand_coz_i_dont_trust_the_mm_subsystem_and_i_dont_know_what_i_want"
I claim that:
- alloc_tx() should only perform the "wait_for_decent_tx_drain" part
- alloc_skb() ought to be done directly in vcc_sendmsg
- alloc_skb() failure can be handled gracefully in vcc_sendmsg
- alloc_skb() may use a (m->msg_flags & MSG_DONTWAIT) dependant
GFP_{KERNEL / ATOMIC} flag
- most of it can be done in a longterm maintenance pain minimizing
way. Call it a side-effect: I don't claim that it *must* be done
this way.
Never disagree, but again, please ensure there is no API brokeness
as I mentioned in the followup patch which you missed. Apparently
my ATM knowledge is not enough to justify the API/ABI.
Thanks.
Were alloc_skb moved one level up in the call stack, there would be
no need to use the new wait api in the subsequent page, thus easing
pre 3.19 longterm kernel maintenance (at least those on korg page).
But it tastes a tad bit too masochistic.
Lack of error handling of allocation failure is always a huge red
flag. We even long ago tried to do something like this for TCP FIN
handling.
It's dumb, it doesn't work.
Therefore I agree that the correct fix is to move the SKB allocation
up one level to vcc_sendmsg() and make it handle errors properly.
If you can justify API is not broken by doing that, I am more than happy
to do it, as I already stated in the latter patch:
"Of course, the logic itself is suspicious, other sendmsg()
could handle skb allocation failure very well, not sure
why ATM has to wait for a successful one here. But probably
it is too late to change since the errno and behavior is
visible to user-space. So just leave the logic as it is."
For some reason, no one reads that patch. :-/
Were alloc_skb moved one level up in the call stack, there would be
no need to use the new wait api in the subsequent page, thus easing
pre 3.19 longterm kernel maintenance (at least those on korg page).
But it tastes a tad bit too masochistic.
Lack of error handling of allocation failure is always a huge red
flag. We even long ago tried to do something like this for TCP FIN
handling.
It's dumb, it doesn't work.
Therefore I agree that the correct fix is to move the SKB allocation
up one level to vcc_sendmsg() and make it handle errors properly.
If you can justify API is not broken by doing that, I am more than happy
to do it, as I already stated in the latter patch:
The man page for sendmsg() allows for ENOMEM. See below.
"Of course, the logic itself is suspicious, other sendmsg()
could handle skb allocation failure very well, not sure
why ATM has to wait for a successful one here. But probably
it is too late to change since the errno and behavior is
visible to user-space. So just leave the logic as it is."
For some reason, no one reads that patch. :-/
I read it and I agree. I think it should be moved up/conflated with
vcc_sendmsg(). vcc_sendmsg() can already return an errno for other
conditions so if so has written something where they are explicitly
not expecting a ENOMEM, we really can't help them.
I would certainly prefer to not have to resort to an atomic allocation.
That's just going to make matters worse as far as similarity to the
existing API.
So, as Francois has suggested, just wait for the atm socket to
drain, and then do the allocation after the wait is finished.
If you can justify API is not broken by doing that, I am more than happy
to do it, as I already stated in the latter patch:
"Of course, the logic itself is suspicious, other sendmsg()
could handle skb allocation failure very well, not sure
why ATM has to wait for a successful one here. But probably
it is too late to change since the errno and behavior is
visible to user-space. So just leave the logic as it is."
For some reason, no one reads that patch. :-/
Believe it or not but I actually read it.
It changes the logic : the original code would have been unable to
escape the while loop on memory failure. Fine, I don't mind the change.
Actually I believe that these two patches are too shy (and backport
unefficient). Instead of trying to reformulate why, here's what I have
in mind. Uncompiled, caveat emptor, etc.
I'll do a (slow) build and test on saturday's night with a pair of
iphase 5575.
On Fri, Jan 13, 2017 at 5:23 AM, Francois Romieu [off-list ref] wrote:
[...]
quoted
alloc_skb() does not need to be in the "while" loop.
This is exactly what I describe in my changelog, don't know
why you want to repeat it...
Because it is still hidden in a while loop.
You turned the alloc from a two level deep "while" loop to a one level
one. I want it at zero level. alloc_skb(..., GFP_KERNEL) fails ?
So let it be done (see patch in other message).
[...]
Please don't expect me to fix many things in one patch, let's
fix each of them separately, agreed?
I am not convinced that several patches are needed to get the whole
picture right.
--
Ueimor
Were alloc_skb moved one level up in the call stack, there would be
no need to use the new wait api in the subsequent page, thus easing
pre 3.19 longterm kernel maintenance (at least those on korg page).
But it tastes a tad bit too masochistic.
Lack of error handling of allocation failure is always a huge red
flag. We even long ago tried to do something like this for TCP FIN
handling.
It's dumb, it doesn't work.
Therefore I agree that the correct fix is to move the SKB allocation
up one level to vcc_sendmsg() and make it handle errors properly.
If you can justify API is not broken by doing that, I am more than happy
to do it, as I already stated in the latter patch:
The man page for sendmsg() allows for ENOMEM. See below.
Errno is just one part, you miss the behavior behind the logic.
quoted
"Of course, the logic itself is suspicious, other sendmsg()
could handle skb allocation failure very well, not sure
why ATM has to wait for a successful one here. But probably
it is too late to change since the errno and behavior is
visible to user-space. So just leave the logic as it is."
For some reason, no one reads that patch. :-/
I read it and I agree. I think it should be moved up/conflated with
vcc_sendmsg(). vcc_sendmsg() can already return an errno for other
conditions so if so has written something where they are explicitly
not expecting a ENOMEM, we really can't help them.
Nope, the reason is never ENOMEM is expected or not. The current
_behavior_ behind this logic might be relied on by user-space.
The behavior here is, when allocation fails, kernel will retry under
certain circumstances, for example, if any fatal signal pending,
returns ERESTARTSYS, etc.. This is what I worry, not just ENOMEM
or not, which is too obvious.
Of course, I could be too conservative, I'd rather not to break things
for -stable at least.
Thanks.
From: Cong Wang <hidden> Date: 2017-01-14 00:36:28
On Fri, Jan 13, 2017 at 4:14 PM, Francois Romieu [off-list ref] wrote:
Cong Wang [off-list ref] :
[...]
quoted
If you can justify API is not broken by doing that, I am more than happy
to do it, as I already stated in the latter patch:
"Of course, the logic itself is suspicious, other sendmsg()
could handle skb allocation failure very well, not sure
why ATM has to wait for a successful one here. But probably
it is too late to change since the errno and behavior is
visible to user-space. So just leave the logic as it is."
For some reason, no one reads that patch. :-/
Believe it or not but I actually read it.
It changes the logic : the original code would have been unable to
escape the while loop on memory failure. Fine, I don't mind the change.
Actually I believe that these two patches are too shy (and backport
unefficient). Instead of trying to reformulate why, here's what I have
in mind. Uncompiled, caveat emptor, etc.
I just don't want to break things, that is it. If you can convince me your
change will not break any user-space application, again I am more
than just happy about it. My ATM knowledge is close to zero. ;)
From: Cong Wang <hidden> Date: 2017-01-14 00:42:15
On Fri, Jan 13, 2017 at 4:15 PM, Francois Romieu [off-list ref] wrote:
Cong Wang [off-list ref] :
quoted
On Fri, Jan 13, 2017 at 5:23 AM, Francois Romieu [off-list ref] wrote:
[...]
quoted
quoted
alloc_skb() does not need to be in the "while" loop.
This is exactly what I describe in my changelog, don't know
why you want to repeat it...
Because it is still hidden in a while loop.
You turned the alloc from a two level deep "while" loop to a one level
one. I want it at zero level. alloc_skb(..., GFP_KERNEL) fails ?
So let it be done (see patch in other message).
Why I didn't remove all the loops is already stated in the later patch,
you said you read it? I doubt. ;)
[...]
quoted
Please don't expect me to fix many things in one patch, let's
fix each of them separately, agreed?
I am not convinced that several patches are needed to get the whole
picture right.
My guideline for stable fixes is one patch fixes one problem, maybe
not suitable to you I think. Let's agree to disagree. ;)
Were alloc_skb moved one level up in the call stack, there would be
no need to use the new wait api in the subsequent page, thus easing
pre 3.19 longterm kernel maintenance (at least those on korg page).
But it tastes a tad bit too masochistic.
Lack of error handling of allocation failure is always a huge red
flag. We even long ago tried to do something like this for TCP FIN
handling.
It's dumb, it doesn't work.
Therefore I agree that the correct fix is to move the SKB allocation
up one level to vcc_sendmsg() and make it handle errors properly.
If you can justify API is not broken by doing that, I am more than happy
to do it, as I already stated in the latter patch:
The man page for sendmsg() allows for ENOMEM. See below.
Errno is just one part, you miss the behavior behind the logic.
quoted
quoted
"Of course, the logic itself is suspicious, other sendmsg()
could handle skb allocation failure very well, not sure
why ATM has to wait for a successful one here. But probably
it is too late to change since the errno and behavior is
visible to user-space. So just leave the logic as it is."
For some reason, no one reads that patch. :-/
I read it and I agree. I think it should be moved up/conflated with
vcc_sendmsg(). vcc_sendmsg() can already return an errno for other
conditions so if so has written something where they are explicitly
not expecting a ENOMEM, we really can't help them.
Nope, the reason is never ENOMEM is expected or not. The current
_behavior_ behind this logic might be relied on by user-space.
The behavior here is, when allocation fails, kernel will retry under
certain circumstances, for example, if any fatal signal pending,
returns ERESTARTSYS, etc.. This is what I worry, not just ENOMEM
or not, which is too obvious.
Yes, and that behavior is certainly wrong. Proving that nothing relies
on it would be very difficult since this is a negative supposition.
It's not clear to me that it is a good idea to ignore the pending signal
and just send the data. At best, this seems like the signal is getting
ignored when the program might actaully want to do something about it.
The way the vcc sockets work, you are almost always waiting for "space
available to send". Since vcc_sendmsg() has always been able to return
ERESTARTSYS for this condition, this isn't exactly new behavior, it
could just happen (very) slightly more often.
The loop in alloc_tx() also ignores the MSG_DONTWAIT flag. The user
might end up waiting after all. So that seems broken as well. If
someone is expecting to wait with MSG_DONTWAIT when memory pressure
is present, I can't help them. They are insane.
Of course, I could be too conservative, I'd rather not to break things
for -stable at least.
Thanks.