Re: [PATCH 07/17] net: convert sock.sk_wmem_alloc from atomic_t to refcount_t

[David Woodhouse <dwmw2@infradead.org>]

[-- Attachment #1: Type: text/plain, Size: 3273 bytes --]



On Fri, 2018-06-15 at 06:39 -0700, Eric Dumazet wrote:
> 
> 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 ?

Yeah... I don't think we want sock_wfree() as a destructor, unless we
also fix up atm_pop_raw() not to do the same refcount_sub() and cope
with some other details, but it could probably be workable with
sufficient caffeine.


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

Right, that was my second suggestion ("copy the accounted value...").

It's a bit of a hack, and I think that actually *using* sock_wfree()
instead of what's currently in atm_pop_raw() would be the better
solution. Does anyone remember why we didn't do that in the first
place?


[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5213 bytes --]