diff mbox series

答复: [PATCH][next-next][v2] netlink: avoid to allocate full skb when sending to many devices

Message ID 2AD939572F25A448A3AE3CAEA61328C237A41FCA@BC-MAIL-M30.internal.baidu.com
State RFC, archived
Delegated to: David Miller
Headers show
Series 答复: [PATCH][next-next][v2] netlink: avoid to allocate full skb when sending to many devices | expand

Commit Message

Li RongQing Sept. 21, 2018, 3:27 a.m. UTC 
: Re: [PATCH][next-next][v2] netlink: avoid to allocate full skb when
> sending to many devices
> 
> 
> 
> On 09/20/2018 06:43 AM, Eric Dumazet wrote:
> >
> 

Sorry, I should cc to you.

> > And lastly this patch looks way too complicated to me.
> > You probably can write something much simpler.
> 

But it  should not increase the negative performance

> Something like :
> 
> diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c index
> 930d17fa906c9ebf1cf7b6031ce0a22f9f66c0e4..e0a81beb4f37751421dbbe794c
> cf3d5a46bdf900 100644
> --- a/net/netlink/af_netlink.c
> +++ b/net/netlink/af_netlink.c
> @@ -278,22 +278,26 @@ static bool netlink_filter_tap(const struct sk_buff
> *skb)
>         return false;
>  }
> 
> -static int __netlink_deliver_tap_skb(struct sk_buff *skb,
> +static int __netlink_deliver_tap_skb(struct sk_buff **pskb,
>                                      struct net_device *dev)  {
> -       struct sk_buff *nskb;
> +       struct sk_buff *nskb, *skb = *pskb;
>         struct sock *sk = skb->sk;
>         int ret = -ENOMEM;
> 
>         if (!net_eq(dev_net(dev), sock_net(sk)))
>                 return 0;
> 
> -       dev_hold(dev);
> -
> -       if (is_vmalloc_addr(skb->head))
> +       if (is_vmalloc_addr(skb->head)) {
>                 nskb = netlink_to_full_skb(skb, GFP_ATOMIC);
> -       else
> -               nskb = skb_clone(skb, GFP_ATOMIC);
> +               if (!nskb)
> +                       return -ENOMEM;
> +               consume_skb(skb);

The original skb can not be freed, since it will be used after send to tap in __netlink_sendskb

> +               skb = nskb;
> +               *pskb = skb;
> +       }
> +       dev_hold(dev);
> +       nskb = skb_clone(skb, GFP_ATOMIC);

since original skb can not be freed, skb_clone will lead to a leak.

>         if (nskb) {
>                 nskb->dev = dev;
>                 nskb->protocol = htons((u16) sk->sk_protocol); @@ -318,7 +322,7
> @@ static void __netlink_deliver_tap(struct sk_buff *skb, struct
> netlink_tap_net *n
>                 return;
> 
>         list_for_each_entry_rcu(tmp, &nn->netlink_tap_all, list) {
> -               ret = __netlink_deliver_tap_skb(skb, tmp->dev);
> +               ret = __netlink_deliver_tap_skb(&skb, tmp->dev);
>                 if (unlikely(ret))
>                         break;
>         }
> 


The below change seems simple, but it increase skb allocation and
free one time,

Comments

Eric Dumazet Sept. 21, 2018, 2:54 p.m. UTC | #1 
On 09/20/2018 08:27 PM, Li,Rongqing wrote:
> 
> The below change seems simple, but it increase skb allocation and
> free one time,  

Seem fine to me. An extra skb_clone() for vmalloc-skb-users is absolute noise,
compared to vmalloc()vfree() cost.

Thanks.
diff mbox series

Patch

diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
index e3a0538ec0be..b9631137f0fe 100644
--- a/net/netlink/af_netlink.c
+++ b/net/netlink/af_netlink.c
@@ -290,10 +290,8 @@  static int __netlink_deliver_tap_skb(struct sk_buff *skb,
 
        dev_hold(dev);
 
-       if (is_vmalloc_addr(skb->head))
-               nskb = netlink_to_full_skb(skb, GFP_ATOMIC);
-       else
-               nskb = skb_clone(skb, GFP_ATOMIC);
+       nskb = skb_clone(skb GFP_ATOMIC);
+
        if (nskb) {
                nskb->dev = dev;
                nskb->protocol = htons((u16) sk->sk_protocol);
@@ -317,11 +315,20 @@  static void __netlink_deliver_tap(struct sk_buff *skb, struct netlink_tap_net *n
        if (!netlink_filter_tap(skb))
                return;
 
+       if (is_vmalloc_addr(skb->head)) {
+               skb = netlink_to_full_skb(skb, GFP_ATOMIC);
+               if (!skb)
+                      return;
+               alloc = true;
+       }
+
        list_for_each_entry_rcu(tmp, &nn->netlink_tap_all, list) {
+
                ret = __netlink_deliver_tap_skb(skb, tmp->dev);
                if (unlikely(ret))
                        break;
        }
+
+       if (alloc)
+               consume_skb(skb);
 }

-Q