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?
smime.p7s
Description: S/MIME cryptographic signature
