From mboxrd@z Thu Jan 1 00:00:00 1970 From: Kevin Darbyshire-Bryant Subject: Re: [PATCH 07/17] net: convert sock.sk_wmem_alloc from atomic_t to refcount_t Date: Fri, 15 Jun 2018 20:49:46 +0000 Message-ID: <32BB74EA-234B-484F-B981-9F3D0027FD82@darbyshire-bryant.me.uk> 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> <3cbab0cc-230d-6980-6748-67f777c8ad22@gmail.com> <1529070283.27158.46.camel@infradead.org> <00520334446ffa4671513bb42ebeeecfab4107e7.camel@infradead.org> Mime-Version: 1.0 Content-Type: multipart/signed; boundary="Apple-Mail=_067FDD1F-F67F-4DA4-941D-D75A48C539CE"; protocol="application/pgp-signature"; micalg=pgp-sha256 Cc: Eric Dumazet , Elena Reshetova , "netdev@vger.kernel.org" , Krzysztof Mazur , "3chas3@gmail.com" <3chas3@gmail.com>, Mathias Kresin To: David Woodhouse Return-path: Received: from mail-eopbgr10041.outbound.protection.outlook.com ([40.107.1.41]:59047 "EHLO EUR02-HE1-obe.outbound.protection.outlook.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1753734AbeFOUtw (ORCPT ); Fri, 15 Jun 2018 16:49:52 -0400 In-Reply-To: <00520334446ffa4671513bb42ebeeecfab4107e7.camel@infradead.org> Content-Language: en-US Sender: netdev-owner@vger.kernel.org List-ID: --Apple-Mail=_067FDD1F-F67F-4DA4-941D-D75A48C539CE Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=utf-8 > On 15 Jun 2018, at 21:00, David Woodhouse wrote: >=20 > On Fri, 2018-06-15 at 14:44 +0100, David Woodhouse wrote: >>=20 >>> Or simply use a new field in ATM_SKB(skb) to remember a stable >>> truesize used in both sides (add/sub) >>=20 >> Right, that was my second suggestion ("copy the accounted value..."). >>=20 >> 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? >=20 > That does end up being quite hairy. I don't think it's worth doing. >=20 > This should probably suffice to fix it... >=20 > Kevin this is going to conflict with the ifx_atm_alloc_skb() hack in > the tree you're working on, but that needs to be killed with fire > anyway. It's utterly pointless as discussed. I had already done so as part of the last pastebin debug info round :-) As regards your patch=E2=80=A6 MAGIC! Works an absolute treat. Will = get that submitted along with the =E2=80=98nuke ifx_atm_alloc_skb=E2=80=99= patch to OpenWrt tomorrow. For now, maybe my brain will let me sleep = :-) Thank you soooooo much for your help & patience. Tested-by: Kevin Darbyshire-Bryant >=20 >=20 > =46rom 3368eaeb0a2f09138894dde0f26f879e5228005a Mon Sep 17 00:00:00 = 2001 > From: David Woodhouse > Date: Fri, 15 Jun 2018 20:49:20 +0100 > Subject: [PATCH] atm: Preserve value of skb->truesize when accounting = to vcc >=20 > There's a hack in pskb_expand_head() to avoid adjusting skb->truesize > for certain skbs. Ideally it would cover ATM too. It doesn't. Just > stashing the accounted value and using it in atm_raw_pop() is probably > the easiest way to cope. >=20 > Signed-off-by: David Woodhouse > --- > include/linux/atmdev.h | 15 +++++++++++++++ > net/atm/br2684.c | 3 +-- > net/atm/clip.c | 3 +-- > net/atm/common.c | 3 +-- > net/atm/lec.c | 3 +-- > net/atm/mpc.c | 3 +-- > net/atm/pppoatm.c | 3 +-- > net/atm/raw.c | 4 ++-- > 8 files changed, 23 insertions(+), 14 deletions(-) >=20 > diff --git a/include/linux/atmdev.h b/include/linux/atmdev.h > index 0c27515d2cf6..8124815eb121 100644 > --- a/include/linux/atmdev.h > +++ b/include/linux/atmdev.h > @@ -214,6 +214,7 @@ struct atmphy_ops { > struct atm_skb_data { > struct atm_vcc *vcc; /* ATM VCC */ > unsigned long atm_options; /* ATM layer options */ > + unsigned int acct_truesize; /* truesize accounted to vcc */ > }; >=20 > #define VCC_HTABLE_SIZE 32 > @@ -241,6 +242,20 @@ void vcc_insert_socket(struct sock *sk); >=20 > void atm_dev_release_vccs(struct atm_dev *dev); >=20 > +static inline void atm_account_tx(struct atm_vcc *vcc, struct sk_buff = *skb) > +{ > + /* > + * Because ATM skbs may not belong to a sock (and we don't > + * necessarily want to), skb->truesize may be adjusted, > + * escaping the hack in pskb_expand_head() which avoids > + * doing so for some cases. So stash the value of truesize > + * at the time we accounted it, and atm_pop_raw() can use > + * that value later, in case it changes. > + */ > + refcount_add(skb->truesize, &sk_atm(vcc)->sk_wmem_alloc); > + ATM_SKB(skb)->acct_truesize =3D skb->truesize; > + ATM_SKB(skb)->atm_options =3D vcc->atm_options; > +} >=20 > static inline void atm_force_charge(struct atm_vcc *vcc,int truesize) > { > diff --git a/net/atm/br2684.c b/net/atm/br2684.c > index 4e111196f902..bc21f8e8daf2 100644 > --- a/net/atm/br2684.c > +++ b/net/atm/br2684.c > @@ -252,8 +252,7 @@ static int br2684_xmit_vcc(struct sk_buff *skb, = struct net_device *dev, >=20 > ATM_SKB(skb)->vcc =3D atmvcc =3D brvcc->atmvcc; > pr_debug("atm_skb(%p)->vcc(%p)->dev(%p)\n", skb, atmvcc, = atmvcc->dev); > - refcount_add(skb->truesize, &sk_atm(atmvcc)->sk_wmem_alloc); > - ATM_SKB(skb)->atm_options =3D atmvcc->atm_options; > + atm_account_tx(atmvcc, skb); > dev->stats.tx_packets++; > dev->stats.tx_bytes +=3D skb->len; >=20 > diff --git a/net/atm/clip.c b/net/atm/clip.c > index 65f706e4344c..60920a42f640 100644 > --- a/net/atm/clip.c > +++ b/net/atm/clip.c > @@ -381,8 +381,7 @@ static netdev_tx_t clip_start_xmit(struct sk_buff = *skb, > memcpy(here, llc_oui, sizeof(llc_oui)); > ((__be16 *) here)[3] =3D skb->protocol; > } > - refcount_add(skb->truesize, &sk_atm(vcc)->sk_wmem_alloc); > - ATM_SKB(skb)->atm_options =3D vcc->atm_options; > + atm_account_tx(vcc, skb); > entry->vccs->last_use =3D jiffies; > pr_debug("atm_skb(%p)->vcc(%p)->dev(%p)\n", skb, vcc, vcc->dev); > old =3D xchg(&entry->vccs->xoff, 1); /* assume XOFF ... */ > diff --git a/net/atm/common.c b/net/atm/common.c > index 8a4f99114cd2..9e812c782a37 100644 > --- a/net/atm/common.c > +++ b/net/atm/common.c > @@ -630,10 +630,9 @@ int vcc_sendmsg(struct socket *sock, struct = msghdr *m, size_t size) > goto out; > } > pr_debug("%d +=3D %d\n", sk_wmem_alloc_get(sk), skb->truesize); > - refcount_add(skb->truesize, &sk->sk_wmem_alloc); > + atm_account_tx(vcc, skb); >=20 > skb->dev =3D NULL; /* for paths shared with net_device = interfaces */ > - ATM_SKB(skb)->atm_options =3D vcc->atm_options; > if (!copy_from_iter_full(skb_put(skb, size), size, = &m->msg_iter)) { > kfree_skb(skb); > error =3D -EFAULT; > diff --git a/net/atm/lec.c b/net/atm/lec.c > index a3d93a1bb133..d7cc165e24e0 100644 > --- a/net/atm/lec.c > +++ b/net/atm/lec.c > @@ -179,9 +179,8 @@ lec_send(struct atm_vcc *vcc, struct sk_buff *skb) > struct net_device *dev =3D skb->dev; >=20 > ATM_SKB(skb)->vcc =3D vcc; > - ATM_SKB(skb)->atm_options =3D vcc->atm_options; > + atm_account_tx(vcc, skb); >=20 > - refcount_add(skb->truesize, &sk_atm(vcc)->sk_wmem_alloc); > if (vcc->send(vcc, skb) < 0) { > dev->stats.tx_dropped++; > return; > diff --git a/net/atm/mpc.c b/net/atm/mpc.c > index 5677147209e8..db9a1838687c 100644 > --- a/net/atm/mpc.c > +++ b/net/atm/mpc.c > @@ -555,8 +555,7 @@ static int send_via_shortcut(struct sk_buff *skb, = struct mpoa_client *mpc) > sizeof(struct llc_snap_hdr)); > } >=20 > - refcount_add(skb->truesize, = &sk_atm(entry->shortcut)->sk_wmem_alloc); > - ATM_SKB(skb)->atm_options =3D entry->shortcut->atm_options; > + atm_account_tx(entry->shortcut, skb); > entry->shortcut->send(entry->shortcut, skb); > entry->packets_fwded++; > mpc->in_ops->put(entry); > diff --git a/net/atm/pppoatm.c b/net/atm/pppoatm.c > index 21d9d341a619..af8c4b38b746 100644 > --- a/net/atm/pppoatm.c > +++ b/net/atm/pppoatm.c > @@ -350,8 +350,7 @@ static int pppoatm_send(struct ppp_channel *chan, = struct sk_buff *skb) > return 1; > } >=20 > - refcount_add(skb->truesize, = &sk_atm(ATM_SKB(skb)->vcc)->sk_wmem_alloc); > - ATM_SKB(skb)->atm_options =3D ATM_SKB(skb)->vcc->atm_options; > + atm_account_tx(vcc, skb); > pr_debug("atm_skb(%p)->vcc(%p)->dev(%p)\n", > skb, ATM_SKB(skb)->vcc, ATM_SKB(skb)->vcc->dev); > ret =3D ATM_SKB(skb)->vcc->send(ATM_SKB(skb)->vcc, skb) > diff --git a/net/atm/raw.c b/net/atm/raw.c > index ee10e8d46185..b3ba44aab0ee 100644 > --- a/net/atm/raw.c > +++ b/net/atm/raw.c > @@ -35,8 +35,8 @@ static void atm_pop_raw(struct atm_vcc *vcc, struct = sk_buff *skb) > struct sock *sk =3D sk_atm(vcc); >=20 > pr_debug("(%d) %d -=3D %d\n", > - vcc->vci, sk_wmem_alloc_get(sk), skb->truesize); > - WARN_ON(refcount_sub_and_test(skb->truesize, = &sk->sk_wmem_alloc)); > + vcc->vci, sk_wmem_alloc_get(sk), = ATM_SKB(skb)->acct_truesize); > + WARN_ON(refcount_sub_and_test(ATM_SKB(skb)->acct_truesize, = &sk->sk_wmem_alloc)); > dev_kfree_skb_any(skb); > sk->sk_write_space(sk); > } > -- > 2.17.0 >=20 > -- > dwmw2 Tested-by: Kevin Darbyshire-Bryant Cheers, Kevin D-B 012C ACB2 28C6 C53E 9775 9123 B3A2 389B 9DE2 334A --Apple-Mail=_067FDD1F-F67F-4DA4-941D-D75A48C539CE Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename=signature.asc Content-Type: application/pgp-signature; name=signature.asc Content-Description: Message signed with OpenPGP -----BEGIN PGP SIGNATURE----- iQIzBAEBCAAdFiEEASyssijGxT6XdZEjs6I4m53iM0oFAlskJmkACgkQs6I4m53i M0peVRAA1wgP4tdQgeIEKdJnr48kQDZ8cEcgwdrX0S0HYIDGS1IEbp4mW+xnfIKo WFds9+Epx+QzAwveuKYsXJN9ZbQK6ShT63dgqoGrVit+MOfObwPqDgGH0YQQ6Xl4 uf2b3n7FTnRBjG+hBjwQeAEVwUcLtUw7SkKVOam5pkuR92G4cIjdbN3Nyx1KfGVN RlFFMXIPgJktnsVOAPIt7QhaNyTEK4fSzB3v2IV68Z5O8esORW3ye8D9wbOoBIpK geUX/SN975Kq0IAxDvZmr8uFjBqnrpVGfeSshFqKnPDzch6ESizmFE1F3QDXERPC eQPwENhhy/ySlFf/Gq0sdFR9pO4oJRUpf/jLHkKwRUu7U1Q26sLfjoj/+dq6q14T 7NPJ/Zj0s0MULcs6IdW3yd46f9PV5zavKqlvx+sgSM9We82+gqAxirydNy96abp9 q3LKNqSCg1z0wJiXlxXdKjQOwB7OiOXunBcI0mu8EfDsCfa0g/MYpmQ+E09pqHkx /U9cZ1P3IURjbe84/TISxJR07KabstBP/J+sMsFSwitFH11R3Dztr3xw0S0VMh2o RtHwfaENTa6BEqDlBu/eQ0avh1fKVI/6Ut1URV5ci+H+Uf+sLL/3WW6EfQsX7Wbo 4It9ommJ2jbTgSxGXOKJYzX5oEMCxA+m0ZcduiwbWuiglUtTl28= =TOR9 -----END PGP SIGNATURE----- --Apple-Mail=_067FDD1F-F67F-4DA4-941D-D75A48C539CE--