[Patch net] atm: remove an unnecessary loop

[Patch net] atm: remove an unnecessary loop

[Cong Wang]

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 <xiyou.wangcong@gmail.com>
---
 net/atm/common.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/net/atm/common.c b/net/atm/common.c
index a3ca922..7ec3bbc 100644
--- a/net/atm/common.c
+++ b/net/atm/common.c
@@ -72,10 +72,11 @@ static struct sk_buff *alloc_tx(struct atm_vcc *vcc, unsigned int size)
 			 sk_wmem_alloc_get(sk), size, sk->sk_sndbuf);
 		return NULL;
 	}
-	while (!(skb = alloc_skb(size, GFP_KERNEL)))
-		schedule();
-	pr_debug("%d += %d\n", sk_wmem_alloc_get(sk), skb->truesize);
-	atomic_add(skb->truesize, &sk->sk_wmem_alloc);
+	skb = alloc_skb(size, GFP_KERNEL);
+	if (skb) {
+		pr_debug("%d += %d\n", sk_wmem_alloc_get(sk), skb->truesize);
+		atomic_add(skb->truesize, &sk->sk_wmem_alloc);
+	}
 	return skb;
 }
 
-- 
2.5.5

[Patch net] atm: switch to the new wait API for vcc_sendmsg()

[Cong Wang]

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 <andreyknvl@google.com>
Tested-by: Andrey Konovalov <andreyknvl@google.com>
Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
---
 net/atm/common.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/net/atm/common.c b/net/atm/common.c
index 7ec3bbc..b672231 100644
--- a/net/atm/common.c
+++ b/net/atm/common.c
@@ -572,8 +572,8 @@ int vcc_recvmsg(struct socket *sock, struct msghdr *msg, size_t size,
 
 int vcc_sendmsg(struct socket *sock, struct msghdr *m, size_t size)
 {
+	DEFINE_WAIT_FUNC(wait, woken_wake_function);
 	struct sock *sk = sock->sk;
-	DEFINE_WAIT(wait);
 	struct atm_vcc *vcc;
 	struct sk_buff *skb;
 	int eff, error;
@@ -605,14 +605,14 @@ int vcc_sendmsg(struct socket *sock, struct msghdr *m, size_t size)
 	}
 
 	eff = (size+3) & ~3; /* align to word boundary */
-	prepare_to_wait(sk_sleep(sk), &wait, TASK_INTERRUPTIBLE);
+	add_wait_queue(sk_sleep(sk), &wait);
 	error = 0;
 	while (!(skb = alloc_tx(vcc, eff))) {
 		if (m->msg_flags & MSG_DONTWAIT) {
 			error = -EAGAIN;
 			break;
 		}
-		schedule();
+		wait_woken(&wait, TASK_INTERRUPTIBLE, MAX_SCHEDULE_TIMEOUT);
 		if (signal_pending(current)) {
 			error = -ERESTARTSYS;
 			break;
@@ -624,9 +624,8 @@ int vcc_sendmsg(struct socket *sock, struct msghdr *m, size_t size)
 			send_sig(SIGPIPE, current, 0);
 			break;
 		}
-		prepare_to_wait(sk_sleep(sk), &wait, TASK_INTERRUPTIBLE);
 	}
-	finish_wait(sk_sleep(sk), &wait);
+	remove_wait_queue(sk_sleep(sk), &wait);
 	if (error)
 		goto out;
 	skb->dev = NULL; /* for paths shared with net_device interfaces */
-- 
2.5.5

Re: [Patch net] atm: remove an unnecessary loop

[Michal Hocko]

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 <xiyou.wangcong@gmail.com>

I cannot give my reviewed-by because I am not familiar with the code but
this looks like an improvement to me.

> ---
>  net/atm/common.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/net/atm/common.c b/net/atm/common.c
> index a3ca922..7ec3bbc 100644
> --- a/net/atm/common.c
> +++ b/net/atm/common.c
> @@ -72,10 +72,11 @@ static struct sk_buff *alloc_tx(struct atm_vcc *vcc, unsigned int size)
>  			 sk_wmem_alloc_get(sk), size, sk->sk_sndbuf);
>  		return NULL;
>  	}
> -	while (!(skb = alloc_skb(size, GFP_KERNEL)))
> -		schedule();
> -	pr_debug("%d += %d\n", sk_wmem_alloc_get(sk), skb->truesize);
> -	atomic_add(skb->truesize, &sk->sk_wmem_alloc);
> +	skb = alloc_skb(size, GFP_KERNEL);
> +	if (skb) {
> +		pr_debug("%d += %d\n", sk_wmem_alloc_get(sk), skb->truesize);
> +		atomic_add(skb->truesize, &sk->sk_wmem_alloc);
> +	}
>  	return skb;
>  }
>  
> -- 
> 2.5.5

-- 
Michal Hocko
SUSE Labs

Re: [Patch net] atm: remove an unnecessary loop

[Francois Romieu]

Cong Wang <xiyou.wangcong@gmail.com> :
[...]
> diff --git a/net/atm/common.c b/net/atm/common.c
> index a3ca922..7ec3bbc 100644
> --- a/net/atm/common.c
> +++ b/net/atm/common.c
> @@ -72,10 +72,11 @@ static struct sk_buff *alloc_tx(struct atm_vcc *vcc, unsigned int size)
>  			 sk_wmem_alloc_get(sk), size, sk->sk_sndbuf);
>  		return NULL;
>  	}
> -	while (!(skb = alloc_skb(size, GFP_KERNEL)))
> -		schedule();
> -	pr_debug("%d += %d\n", sk_wmem_alloc_get(sk), skb->truesize);
> -	atomic_add(skb->truesize, &sk->sk_wmem_alloc);
> +	skb = alloc_skb(size, GFP_KERNEL);
> +	if (skb) {
> +		pr_debug("%d += %d\n", sk_wmem_alloc_get(sk), skb->truesize);
> +		atomic_add(skb->truesize, &sk->sk_wmem_alloc);
> +	}
>  	return skb;
>  }

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

Re: [Patch net] atm: remove an unnecessary loop

[Cong Wang]

On Thu, Jan 12, 2017 at 4:07 PM, Francois Romieu <romieu@fr.zoreil.com> wrote:
> Cong Wang <xiyou.wangcong@gmail.com> :
> [...]
>> diff --git a/net/atm/common.c b/net/atm/common.c
>> index a3ca922..7ec3bbc 100644
>> --- a/net/atm/common.c
>> +++ b/net/atm/common.c
>> @@ -72,10 +72,11 @@ static struct sk_buff *alloc_tx(struct atm_vcc *vcc, unsigned int size)
>>                        sk_wmem_alloc_get(sk), size, sk->sk_sndbuf);
>>               return NULL;
>>       }
>> -     while (!(skb = alloc_skb(size, GFP_KERNEL)))
>> -             schedule();
>> -     pr_debug("%d += %d\n", sk_wmem_alloc_get(sk), skb->truesize);
>> -     atomic_add(skb->truesize, &sk->sk_wmem_alloc);
>> +     skb = alloc_skb(size, GFP_KERNEL);
>> +     if (skb) {
>> +             pr_debug("%d += %d\n", sk_wmem_alloc_get(sk), skb->truesize);
>> +             atomic_add(skb->truesize, &sk->sk_wmem_alloc);
>> +     }
>>       return skb;
>>  }
>
> 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.

Re: [Patch net] atm: remove an unnecessary loop

[Francois Romieu]

Cong Wang <xiyou.wangcong@gmail.com> :
[...]
> 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

Re: [Patch net] atm: remove an unnecessary loop

[David Miller]

From: Francois Romieu <romieu@fr.zoreil.com>
Date: Fri, 13 Jan 2017 01:07:00 +0100

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

Re: [Patch net] atm: remove an unnecessary loop

[Cong Wang]

On Fri, Jan 13, 2017 at 5:23 AM, Francois Romieu <romieu@fr.zoreil.com> wrote:
> Cong Wang <xiyou.wangcong@gmail.com> :
> [...]
>> 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.

Re: [Patch net] atm: remove an unnecessary loop

[Cong Wang]

On Fri, Jan 13, 2017 at 9:10 AM, David Miller <davem@davemloft.net> wrote:
> From: Francois Romieu <romieu@fr.zoreil.com>
> Date: Fri, 13 Jan 2017 01:07:00 +0100
>
>> 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. :-/

Re: [Patch net] atm: remove an unnecessary loop

[Chas Williams]

On Fri, 2017-01-13 at 10:20 -0800, Cong Wang wrote:
> On Fri, Jan 13, 2017 at 9:10 AM, David Miller <davem@davemloft.net> wrote:
> > From: Francois Romieu <romieu@fr.zoreil.com>
> > Date: Fri, 13 Jan 2017 01:07:00 +0100
> >
> >> 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.

Re: [Patch net] atm: remove an unnecessary loop

[Francois Romieu]

Cong Wang <xiyou.wangcong@gmail.com> :
[...]
> 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.

diff --git a/net/atm/common.c b/net/atm/common.c
index a3ca922..67f76f3 100644
--- a/net/atm/common.c
+++ b/net/atm/common.c
@@ -62,21 +62,16 @@ static void vcc_remove_socket(struct sock *sk)
 	write_unlock_irq(&vcc_sklist_lock);
 }
 
-static struct sk_buff *alloc_tx(struct atm_vcc *vcc, unsigned int size)
+static bool vcc_tx_ready(struct atm_vcc *vcc, unsigned int size)
 {
-	struct sk_buff *skb;
 	struct sock *sk = sk_atm(vcc);
 
 	if (sk_wmem_alloc_get(sk) && !atm_may_send(vcc, size)) {
 		pr_debug("Sorry: wmem_alloc = %d, size = %d, sndbuf = %d\n",
 			 sk_wmem_alloc_get(sk), size, sk->sk_sndbuf);
-		return NULL;
+		return false;
 	}
-	while (!(skb = alloc_skb(size, GFP_KERNEL)))
-		schedule();
-	pr_debug("%d += %d\n", sk_wmem_alloc_get(sk), skb->truesize);
-	atomic_add(skb->truesize, &sk->sk_wmem_alloc);
-	return skb;
+	return true;
 }
 
 static void vcc_sock_destruct(struct sock *sk)
@@ -606,7 +601,7 @@ int vcc_sendmsg(struct socket *sock, struct msghdr *m, size_t size)
 	eff = (size+3) & ~3; /* align to word boundary */
 	prepare_to_wait(sk_sleep(sk), &wait, TASK_INTERRUPTIBLE);
 	error = 0;
-	while (!(skb = alloc_tx(vcc, eff))) {
+	while (!vcc_tx_ready(vcc, eff)) {
 		if (m->msg_flags & MSG_DONTWAIT) {
 			error = -EAGAIN;
 			break;
@@ -628,6 +623,13 @@ int vcc_sendmsg(struct socket *sock, struct msghdr *m, size_t size)
 	finish_wait(sk_sleep(sk), &wait);
 	if (error)
 		goto out;
+
+	skb = alloc_skb(eff, GFP_KERNEL);
+	if (!skb)
+		goto out;
+	pr_debug("%d += %d\n", sk_wmem_alloc_get(sk), skb->truesize);
+	atomic_add(skb->truesize, &sk->sk_wmem_alloc);
+
 	skb->dev = NULL; /* for paths shared with net_device interfaces */
 	ATM_SKB(skb)->atm_options = vcc->atm_options;
 	if (!copy_from_iter_full(skb_put(skb, size), size, &m->msg_iter)) {

-- 
Ueimor

Re: [Patch net] atm: remove an unnecessary loop

[Francois Romieu]

Cong Wang <xiyou.wangcong@gmail.com> :
> On Fri, Jan 13, 2017 at 5:23 AM, Francois Romieu <romieu@fr.zoreil.com> wrote:
[...]
> > 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

Re: [Patch net] atm: remove an unnecessary loop

[Francois Romieu]

Francois Romieu <romieu@fr.zoreil.com> :
[...]

Now with a proper error code. Have a nice night.

diff --git a/net/atm/common.c b/net/atm/common.c
index a3ca922..e20d040 100644
--- a/net/atm/common.c
+++ b/net/atm/common.c
@@ -62,21 +62,16 @@ static void vcc_remove_socket(struct sock *sk)
 	write_unlock_irq(&vcc_sklist_lock);
 }
 
-static struct sk_buff *alloc_tx(struct atm_vcc *vcc, unsigned int size)
+static bool vcc_tx_ready(struct atm_vcc *vcc, unsigned int size)
 {
-	struct sk_buff *skb;
 	struct sock *sk = sk_atm(vcc);
 
 	if (sk_wmem_alloc_get(sk) && !atm_may_send(vcc, size)) {
 		pr_debug("Sorry: wmem_alloc = %d, size = %d, sndbuf = %d\n",
 			 sk_wmem_alloc_get(sk), size, sk->sk_sndbuf);
-		return NULL;
+		return false;
 	}
-	while (!(skb = alloc_skb(size, GFP_KERNEL)))
-		schedule();
-	pr_debug("%d += %d\n", sk_wmem_alloc_get(sk), skb->truesize);
-	atomic_add(skb->truesize, &sk->sk_wmem_alloc);
-	return skb;
+	return true;
 }
 
 static void vcc_sock_destruct(struct sock *sk)
@@ -606,7 +601,7 @@ int vcc_sendmsg(struct socket *sock, struct msghdr *m, size_t size)
 	eff = (size+3) & ~3; /* align to word boundary */
 	prepare_to_wait(sk_sleep(sk), &wait, TASK_INTERRUPTIBLE);
 	error = 0;
-	while (!(skb = alloc_tx(vcc, eff))) {
+	while (!vcc_tx_ready(vcc, eff)) {
 		if (m->msg_flags & MSG_DONTWAIT) {
 			error = -EAGAIN;
 			break;
@@ -628,6 +623,15 @@ int vcc_sendmsg(struct socket *sock, struct msghdr *m, size_t size)
 	finish_wait(sk_sleep(sk), &wait);
 	if (error)
 		goto out;
+
+	skb = alloc_skb(eff, GFP_KERNEL);
+	if (!skb) {
+		error = -ENOMEM;
+		goto out;
+	}
+	pr_debug("%d += %d\n", sk_wmem_alloc_get(sk), skb->truesize);
+	atomic_add(skb->truesize, &sk->sk_wmem_alloc);
+
 	skb->dev = NULL; /* for paths shared with net_device interfaces */
 	ATM_SKB(skb)->atm_options = vcc->atm_options;
 	if (!copy_from_iter_full(skb_put(skb, size), size, &m->msg_iter)) {

Re: [Patch net] atm: remove an unnecessary loop

[Cong Wang]

On Fri, Jan 13, 2017 at 3:54 PM, Chas Williams <3chas3@gmail.com> wrote:
> On Fri, 2017-01-13 at 10:20 -0800, Cong Wang wrote:
>> On Fri, Jan 13, 2017 at 9:10 AM, David Miller <davem@davemloft.net> wrote:
>> > From: Francois Romieu <romieu@fr.zoreil.com>
>> > Date: Fri, 13 Jan 2017 01:07:00 +0100
>> >
>> >> 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.

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

Re: [Patch net] atm: remove an unnecessary loop

[Cong Wang]

On Fri, Jan 13, 2017 at 4:14 PM, Francois Romieu <romieu@fr.zoreil.com> wrote:
> Cong Wang <xiyou.wangcong@gmail.com> :
> [...]
>> 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. ;)

Re: [Patch net] atm: remove an unnecessary loop

[Cong Wang]

On Fri, Jan 13, 2017 at 4:15 PM, Francois Romieu <romieu@fr.zoreil.com> wrote:
> Cong Wang <xiyou.wangcong@gmail.com> :
>> On Fri, Jan 13, 2017 at 5:23 AM, Francois Romieu <romieu@fr.zoreil.com> wrote:
> [...]
>> > 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. ;)


> [...]
>> 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. ;)

Re: [Patch net] atm: remove an unnecessary loop

[Chas Williams]

On Fri, 2017-01-13 at 16:30 -0800, Cong Wang wrote:
> On Fri, Jan 13, 2017 at 3:54 PM, Chas Williams <3chas3@gmail.com> wrote:
> > On Fri, 2017-01-13 at 10:20 -0800, Cong Wang wrote:
> >> On Fri, Jan 13, 2017 at 9:10 AM, David Miller <davem@davemloft.net> wrote:
> >> > From: Francois Romieu <romieu@fr.zoreil.com>
> >> > Date: Fri, 13 Jan 2017 01:07:00 +0100
> >> >
> >> >> 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.
> 
> >>
> >> "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.