From mboxrd@z Thu Jan 1 00:00:00 1970 From: Eric Dumazet Subject: Re: [PATCH 07/17] net: convert sock.sk_wmem_alloc from atomic_t to refcount_t Date: Fri, 15 Jun 2018 06:27:46 -0700 Message-ID: <9805cd6e-1c21-f7b4-cc0e-d4a343b8e3d8@gmail.com> References: <1498817290-3368-1-git-send-email-elena.reshetova@intel.com> <1498817290-3368-8-git-send-email-elena.reshetova@intel.com> <1529065762.27158.40.camel@infradead.org> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Cc: Krzysztof Mazur , Kevin Darbyshire-Bryant , chas@cmf.nrl.navy.mil, Mathias Kresin To: David Woodhouse , Elena Reshetova , netdev@vger.kernel.org Return-path: Received: from mail-pl0-f67.google.com ([209.85.160.67]:35092 "EHLO mail-pl0-f67.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S933771AbeFON1t (ORCPT ); Fri, 15 Jun 2018 09:27:49 -0400 Received: by mail-pl0-f67.google.com with SMTP id k1-v6so5390159plt.2 for ; Fri, 15 Jun 2018 06:27:48 -0700 (PDT) In-Reply-To: <1529065762.27158.40.camel@infradead.org> Content-Language: en-US Sender: netdev-owner@vger.kernel.org List-ID: On 06/15/2018 05:29 AM, David Woodhouse wrote: > On Fri, 2017-06-30 at 13:08 +0300, Elena Reshetova wrote: >> diff --git a/include/linux/atmdev.h b/include/linux/atmdev.h >> index c1da539..4d97a89 100644 >> --- a/include/linux/atmdev.h >> +++ b/include/linux/atmdev.h >> @@ -254,7 +254,7 @@ static inline void atm_return(struct atm_vcc *vcc,int truesize) >>   >>  static inline int atm_may_send(struct atm_vcc *vcc,unsigned int size) >>  { >> -       return (size + atomic_read(&sk_atm(vcc)->sk_wmem_alloc)) < >> +       return (size + refcount_read(&sk_atm(vcc)->sk_wmem_alloc)) < >>                sk_atm(vcc)->sk_sndbuf; >>  } > > > Hm, this (commit 14afee4b6092fd) may have broken PPPoATM. I did spend a > while staring hard at my own commit 9d02daf754238 which introduced > pppoatm_may_send(), but it's actually atm_may_send() which is never > allowing packets to be pushed. > > Debugging (which is ongoing) has so far shown that we are accounting > for a packet in pppoatm_send() which has skb->truesize==0x1c0, and > later end up in pppoatm_pop()→atm_raw_pop() with skb->truesize==0x2c0. > > This was always harmless before, but now it's a refcount_t it appears > to underflow and go into its "screw you" mode and never let any more > packets get sent. > > I'm staring hard at the special case in pskb_expand_head() to *not* > change skb->truesize under certain circumstances, and wondering if that > (is, should be) covering the case of ATM skbs: > >         /* It is not generally safe to change skb->truesize. >          * For the moment, we really care of rx path, or >          * when skb is orphaned (not attached to a socket). >          */ >         if (!skb->sk || skb->destructor == sock_edemux) >                 skb->truesize += size - osize; > > Failing that, maybe we should copy the accounted value of skb->truesize > into the struct skb_atm_data in skb->cb at the time we add it to > sk_wmem_alloc — and then subtract *that* value from sk_wmem_alloc in > atm_raw_pop() instead of the then current value of skb->truesize. > > Suggestions? > Maybe ATM should make sure skb->sk is set ? something like the following : diff --git a/net/atm/pppoatm.c b/net/atm/pppoatm.c index 21d9d341a6199255a017437954e4b688f1ba5bfd..4863d02939a26ce0a5816da86779336255d550bd 100644 --- a/net/atm/pppoatm.c +++ b/net/atm/pppoatm.c @@ -350,7 +350,7 @@ static int pppoatm_send(struct ppp_channel *chan, struct sk_buff *skb) return 1; } - refcount_add(skb->truesize, &sk_atm(ATM_SKB(skb)->vcc)->sk_wmem_alloc); + skb_set_owner_w(skb, &sk_atm(ATM_SKB(skb)->vcc)); ATM_SKB(skb)->atm_options = ATM_SKB(skb)->vcc->atm_options; pr_debug("atm_skb(%p)->vcc(%p)->dev(%p)\n", skb, ATM_SKB(skb)->vcc, ATM_SKB(skb)->vcc->dev); Or something more sophisticated, but this might need more thinking diff --git a/net/atm/pppoatm.c b/net/atm/pppoatm.c index 21d9d341a6199255a017437954e4b688f1ba5bfd..80902a0b56a1d15d2851a07f067490d99136d214 100644 --- a/net/atm/pppoatm.c +++ b/net/atm/pppoatm.c @@ -350,7 +350,10 @@ static int pppoatm_send(struct ppp_channel *chan, struct sk_buff *skb) return 1; } - refcount_add(skb->truesize, &sk_atm(ATM_SKB(skb)->vcc)->sk_wmem_alloc); + if (!skb->sk) + skb_set_owner_w(skb, &sk_atm(ATM_SKB(skb)->vcc)); +- else + refcount_add(skb->truesize, &sk_atm(ATM_SKB(skb)->vcc)->sk_wmem_alloc); ATM_SKB(skb)->atm_options = ATM_SKB(skb)->vcc->atm_options; pr_debug("atm_skb(%p)->vcc(%p)->dev(%p)\n", skb, ATM_SKB(skb)->vcc, ATM_SKB(skb)->vcc->dev);