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:39:15 -0700 Message-ID: <3cbab0cc-230d-6980-6748-67f777c8ad22@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> <9805cd6e-1c21-f7b4-cc0e-d4a343b8e3d8@gmail.com> 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: Eric Dumazet , David Woodhouse , Elena Reshetova , netdev@vger.kernel.org Return-path: Received: from mail-pg0-f66.google.com ([74.125.83.66]:39145 "EHLO mail-pg0-f66.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S936211AbeFONjS (ORCPT ); Fri, 15 Jun 2018 09:39:18 -0400 Received: by mail-pg0-f66.google.com with SMTP id w12-v6so4458124pgc.6 for ; Fri, 15 Jun 2018 06:39:18 -0700 (PDT) In-Reply-To: <9805cd6e-1c21-f7b4-cc0e-d4a343b8e3d8@gmail.com> Content-Language: en-US Sender: netdev-owner@vger.kernel.org List-ID: On 06/15/2018 06:27 AM, Eric Dumazet wrote: > > > 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 : > Or simply use a new field in ATM_SKB(skb) to remember a stable truesize used in both sides (add/sub)