> On 15 Jun 2018, at 21:00, David Woodhouse <dw...@infradead.org> wrote:
>
> On Fri, 2018-06-15 at 14:44 +0100, David Woodhouse wrote:
>>
>>> 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?
>
> That does end up being quite hairy. I don't think it's worth doing.
>
> This should probably suffice to fix it...
>
> 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… MAGIC! Works an absolute treat. Will get that
submitted along with the ‘nuke ifx_atm_alloc_skb’ 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 <l...@darbyshire-bryant.me.uk>
>
>
> From 3368eaeb0a2f09138894dde0f26f879e5228005a Mon Sep 17 00:00:00 2001
> From: David Woodhouse <dw...@infradead.org>
> Date: Fri, 15 Jun 2018 20:49:20 +0100
> Subject: [PATCH] atm: Preserve value of skb->truesize when accounting to vcc
>
> 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.
>
> Signed-off-by: David Woodhouse <dw...@infradead.org>
> ---
> 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(-)
>
> 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 */
> };
>
> #define VCC_HTABLE_SIZE 32
> @@ -241,6 +242,20 @@ void vcc_insert_socket(struct sock *sk);
>
> void atm_dev_release_vccs(struct atm_dev *dev);
>
> +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 = skb->truesize;
> + ATM_SKB(skb)->atm_options = vcc->atm_options;
> +}
>
> 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,
>
> ATM_SKB(skb)->vcc = atmvcc = 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 = atmvcc->atm_options;
> + atm_account_tx(atmvcc, skb);
> dev->stats.tx_packets++;
> dev->stats.tx_bytes += skb->len;
>
> 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] = skb->protocol;
> }
> - refcount_add(skb->truesize, &sk_atm(vcc)->sk_wmem_alloc);
> - ATM_SKB(skb)->atm_options = vcc->atm_options;
> + atm_account_tx(vcc, skb);
> entry->vccs->last_use = jiffies;
> pr_debug("atm_skb(%p)->vcc(%p)->dev(%p)\n", skb, vcc, vcc->dev);
> old = 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 += %d\n", sk_wmem_alloc_get(sk), skb->truesize);
> - refcount_add(skb->truesize, &sk->sk_wmem_alloc);
> + atm_account_tx(vcc, skb);
>
> skb->dev = NULL; /* for paths shared with net_device interfaces */
> - ATM_SKB(skb)->atm_options = vcc->atm_options;
> if (!copy_from_iter_full(skb_put(skb, size), size, &m->msg_iter)) {
> kfree_skb(skb);
> error = -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 = skb->dev;
>
> ATM_SKB(skb)->vcc = vcc;
> - ATM_SKB(skb)->atm_options = vcc->atm_options;
> + atm_account_tx(vcc, skb);
>
> - 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));
> }
>
> - refcount_add(skb->truesize, &sk_atm(entry->shortcut)->sk_wmem_alloc);
> - ATM_SKB(skb)->atm_options = 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;
> }
>
> - refcount_add(skb->truesize, &sk_atm(ATM_SKB(skb)->vcc)->sk_wmem_alloc);
> - ATM_SKB(skb)->atm_options = 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 = 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 = sk_atm(vcc);
>
> pr_debug("(%d) %d -= %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
>
> --
> dwmw2
Tested-by: Kevin Darbyshire-Bryant <l...@darbyshire-bryant.me.uk>
Cheers,
Kevin D-B
012C ACB2 28C6 C53E 9775 9123 B3A2 389B 9DE2 334A
signature.asc
Description: Message signed with OpenPGP
