[PATCH net-next] atm: use SKB_TRUESIZE() in atm_guess_pdu2truesize()

[PATCH net-next] atm: use SKB_TRUESIZE() in atm_guess_pdu2truesize()

[Eric Dumazet]

SKB_TRUESIZE() provides a better approximation of expected skb truesize.

Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
---
 include/linux/atmdev.h |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/atmdev.h b/include/linux/atmdev.h
index 49a83ca..43ea1b2 100644
--- a/include/linux/atmdev.h
+++ b/include/linux/atmdev.h
@@ -452,7 +452,7 @@ void atm_dev_release_vccs(struct atm_dev *dev);
 
 static inline int atm_guess_pdu2truesize(int size)
 {
-	return SKB_DATA_ALIGN(size) + sizeof(struct skb_shared_info);
+	return SKB_TRUESIZE(size);
 }
 
 

Re: [PATCH net-next] atm: use SKB_TRUESIZE() in atm_guess_pdu2truesize()

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Tue, 22 Nov 2011 06:51:34 +0100

> SKB_TRUESIZE() provides a better approximation of expected skb truesize.
> 
> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>

Applied.

Re: [PATCH net-next] atm: use SKB_TRUESIZE() in atm_guess_pdu2truesize()

[chas williams - CONTRACTOR]

it doesnt seem like a good idea to create a wrapper for a one to one
call.  perhaps this whole bit of nonsense should be removed.  the
iphase driver should be returning skb->truesize like everyone.  

if atm_alloc_charge() just uses SKB_TRUESIZE() then we konw that guess
will be the same as skb->truesize and atm_alloc_charge() can be
simplified by removing atomic_add(skb->truesize - guess, which will be
0 in all cases.

something like:

diff --git a/drivers/atm/iphase.c b/drivers/atm/iphase.c
index 3d0c2b0..9e373ba 100644
--- a/drivers/atm/iphase.c
+++ b/drivers/atm/iphase.c
@@ -1320,8 +1320,8 @@ static void rx_dle_intr(struct atm_dev *dev)
           if (ia_vcc == NULL)
           {
              atomic_inc(&vcc->stats->rx_err);
+             atm_return(vcc, skb->truesize);
              dev_kfree_skb_any(skb);
-             atm_return(vcc, atm_guess_pdu2truesize(len));
              goto INCR_DLE;
            }
           // get real pkt length  pwang_test
@@ -1334,8 +1334,8 @@ static void rx_dle_intr(struct atm_dev *dev)
              atomic_inc(&vcc->stats->rx_err);
              IF_ERR(printk("rx_dle_intr: Bad  AAL5 trailer %d (skb len %d)", 
                                                             length, skb->len);)
+             atm_return(vcc, skb->truesize);
              dev_kfree_skb_any(skb);
-             atm_return(vcc, atm_guess_pdu2truesize(len));
              goto INCR_DLE;
           }
           skb_trim(skb, length);
diff --git a/net/atm/atm_misc.c b/net/atm/atm_misc.c
index f41f026..073fd99 100644
--- a/net/atm/atm_misc.c
+++ b/net/atm/atm_misc.c
@@ -26,19 +26,16 @@ struct sk_buff *atm_alloc_charge(struct atm_vcc *vcc, int pdu_size,
                                 gfp_t gfp_flags)
 {
        struct sock *sk = sk_atm(vcc);
-       int guess = atm_guess_pdu2truesize(pdu_size);
+       int truesize = SKB_TRUESIZE(pdu_size);
 
-       atm_force_charge(vcc, guess);
+       atm_force_charge(vcc, truesize);
        if (atomic_read(&sk->sk_rmem_alloc) <= sk->sk_rcvbuf) {
                struct sk_buff *skb = alloc_skb(pdu_size, gfp_flags);
 
-               if (skb) {
-                       atomic_add(skb->truesize-guess,
-                                  &sk->sk_rmem_alloc);
+               if (skb)
                        return skb;
-               }
        }
-       atm_return(vcc, guess);
+       atm_return(vcc, truesize);
        atomic_inc(&vcc->stats->rx_drop);
        return NULL;
 }


On Tue, 22 Nov 2011 06:51:34 +0100
Eric Dumazet <eric.dumazet@gmail.com> wrote:

> SKB_TRUESIZE() provides a better approximation of expected skb truesize.
> 
> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
> ---
>  include/linux/atmdev.h |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/include/linux/atmdev.h b/include/linux/atmdev.h
> index 49a83ca..43ea1b2 100644
> --- a/include/linux/atmdev.h
> +++ b/include/linux/atmdev.h
> @@ -452,7 +452,7 @@ void atm_dev_release_vccs(struct atm_dev *dev);
>  
>  static inline int atm_guess_pdu2truesize(int size)
>  {
> -	return SKB_DATA_ALIGN(size) + sizeof(struct skb_shared_info);
> +	return SKB_TRUESIZE(size);
>  }
>  
>  
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH net-next] atm: use SKB_TRUESIZE() in atm_guess_pdu2truesize()

[Eric Dumazet]

Le mardi 22 novembre 2011 à 15:22 -0500, chas williams - CONTRACTOR a
écrit :
> it doesnt seem like a good idea to create a wrapper for a one to one
> call.  perhaps this whole bit of nonsense should be removed.  the
> iphase driver should be returning skb->truesize like everyone.  
> 
> if atm_alloc_charge() just uses SKB_TRUESIZE() then we konw that guess
> will be the same as skb->truesize and atm_alloc_charge() can be
> simplified by removing atomic_add(skb->truesize - guess, which will be
> 0 in all cases.
> 

Please note I didnt create a wrapper, only correct existing one :)

Feel free to send a (tested) patch, but be warned that following code is
not correct :

int size = something;

struct sk_buff *skb = skb_alloc(size);

ASSERT(skb->truesize == SKB_TRUESIZE(size));

(It might be true with SLOB only)

Re: [PATCH net-next] atm: use SKB_TRUESIZE() in atm_guess_pdu2truesize()

[chas williams - CONTRACTOR]

On Tue, 22 Nov 2011 21:33:54 +0100
Eric Dumazet <eric.dumazet@gmail.com> wrote:

> Le mardi 22 novembre 2011 à 15:22 -0500, chas williams - CONTRACTOR a
> écrit :
> > it doesnt seem like a good idea to create a wrapper for a one to one
> > call.  perhaps this whole bit of nonsense should be removed.  the
> > iphase driver should be returning skb->truesize like everyone.  
> > 
> > if atm_alloc_charge() just uses SKB_TRUESIZE() then we konw that guess
> > will be the same as skb->truesize and atm_alloc_charge() can be
> > simplified by removing atomic_add(skb->truesize - guess, which will be
> > 0 in all cases.
> > 
> 
> Please note I didnt create a wrapper, only correct existing one :)
> 
> Feel free to send a (tested) patch, but be warned that following code is
> not correct :
> 
> int size = something;
> 
> struct sk_buff *skb = skb_alloc(size);
> 
> ASSERT(skb->truesize == SKB_TRUESIZE(size));
> 
> (It might be true with SLOB only)

perhaps it is best to say that you discovered an inadvertent wrapper.

given skb->truesize != SKB_TRUESIZE() for all cases, it is certainly
wrong in the iphase driver to return the guessed value.  it should be
returning skb->truesize.  i will send along a patch shortly to just get
rid of this function.