All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH][next-next][v2] netlink: avoid to allocate full skb when sending to many devices
@ 2018-09-20  8:54 Li RongQing
  2018-09-20 13:43 ` Eric Dumazet
  2018-09-21 19:08 ` Cong Wang
  0 siblings, 2 replies; 6+ messages in thread
From: Li RongQing @ 2018-09-20  8:54 UTC (permalink / raw)
  To: netdev

if skb->head is vmalloc address, when this skb is delivered, full
allocation for this skb is required, if there are many devices,
the full allocation will be called for every devices

now if it is vmalloc, allocate a new skb, whose data is not vmalloc
address, and use new allocated skb to clone and send, to avoid to
allocate full skb everytime.

Signed-off-by: Zhang Yu <zhangyu31@baidu.com>
Signed-off-by: Li RongQing <lirongqing@baidu.com>
---
 net/netlink/af_netlink.c | 37 ++++++++++++++++++++++++++++++-------
 1 file changed, 30 insertions(+), 7 deletions(-)

diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
index e3a0538ec0be..a5b1bf706526 100644
--- a/net/netlink/af_netlink.c
+++ b/net/netlink/af_netlink.c
@@ -279,21 +279,25 @@ static bool netlink_filter_tap(const struct sk_buff *skb)
 }
 
 static int __netlink_deliver_tap_skb(struct sk_buff *skb,
-				     struct net_device *dev)
+				 struct net_device *dev, bool alloc, bool last)
 {
 	struct sk_buff *nskb;
 	struct sock *sk = skb->sk;
 	int ret = -ENOMEM;
 
-	if (!net_eq(dev_net(dev), sock_net(sk)))
+	if (!net_eq(dev_net(dev), sock_net(sk))) {
+		if (last && alloc)
+			consume_skb(skb);
 		return 0;
+	}
 
 	dev_hold(dev);
 
-	if (is_vmalloc_addr(skb->head))
-		nskb = netlink_to_full_skb(skb, GFP_ATOMIC);
+	if (unlikely(last && alloc))
+		nskb = skb;
 	else
 		nskb = skb_clone(skb, GFP_ATOMIC);
+
 	if (nskb) {
 		nskb->dev = dev;
 		nskb->protocol = htons((u16) sk->sk_protocol);
@@ -303,6 +307,8 @@ static int __netlink_deliver_tap_skb(struct sk_buff *skb,
 		ret = dev_queue_xmit(nskb);
 		if (unlikely(ret > 0))
 			ret = net_xmit_errno(ret);
+	} else if (alloc) {
+		kfree_skb(skb);
 	}
 
 	dev_put(dev);
@@ -311,16 +317,33 @@ static int __netlink_deliver_tap_skb(struct sk_buff *skb,
 
 static void __netlink_deliver_tap(struct sk_buff *skb, struct netlink_tap_net *nn)
 {
+	struct netlink_tap *tmp, *next;
+	bool alloc = false;
 	int ret;
-	struct netlink_tap *tmp;
 
 	if (!netlink_filter_tap(skb))
 		return;
 
-	list_for_each_entry_rcu(tmp, &nn->netlink_tap_all, list) {
-		ret = __netlink_deliver_tap_skb(skb, tmp->dev);
+	tmp = list_first_or_null_rcu(&nn->netlink_tap_all,
+					struct netlink_tap, list);
+	if (!tmp)
+		return;
+
+	if (is_vmalloc_addr(skb->head)) {
+		skb = netlink_to_full_skb(skb, GFP_ATOMIC);
+		if (!skb)
+			return;
+		alloc = true;
+	}
+
+	while (tmp) {
+		next = list_next_or_null_rcu(&nn->netlink_tap_all, &tmp->list,
+					struct netlink_tap, list);
+
+		ret = __netlink_deliver_tap_skb(skb, tmp->dev, alloc, !next);
 		if (unlikely(ret))
 			break;
+		tmp = next;
 	}
 }
 
-- 
2.16.2

^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH][next-next][v2] netlink: avoid to allocate full skb when sending to many devices
  2018-09-20  8:54 [PATCH][next-next][v2] netlink: avoid to allocate full skb when sending to many devices Li RongQing
@ 2018-09-20 13:43 ` Eric Dumazet
  2018-09-20 13:51   ` Eric Dumazet
  2018-09-21 19:08 ` Cong Wang
  1 sibling, 1 reply; 6+ messages in thread
From: Eric Dumazet @ 2018-09-20 13:43 UTC (permalink / raw)
  To: Li RongQing, netdev



On 09/20/2018 01:54 AM, Li RongQing wrote:
> if skb->head is vmalloc address, when this skb is delivered, full
> allocation for this skb is required, if there are many devices,
> the full allocation will be called for every devices
> 
> now if it is vmalloc, allocate a new skb, whose data is not vmalloc
> address, and use new allocated skb to clone and send, to avoid to
> allocate full skb everytime.
> 
> Signed-off-by: Zhang Yu <zhangyu31@baidu.com>
> Signed-off-by: Li RongQing <lirongqing@baidu.com>


This looks a broken Signed-off-by chain to me.

Who really wrote this patch ?

Also, given that I gave feedback to your first version,
it is customary to CC me for future versions, unless you want me to not notice them.

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

Thanks.

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH][next-next][v2] netlink: avoid to allocate full skb when sending to many devices
  2018-09-20 13:43 ` Eric Dumazet
@ 2018-09-20 13:51   ` Eric Dumazet
  2018-09-21  3:27     ` 答复: " Li,Rongqing
  0 siblings, 1 reply; 6+ messages in thread
From: Eric Dumazet @ 2018-09-20 13:51 UTC (permalink / raw)
  To: Li RongQing, netdev



On 09/20/2018 06:43 AM, Eric Dumazet wrote:
> 

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

Something like :

diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
index 930d17fa906c9ebf1cf7b6031ce0a22f9f66c0e4..e0a81beb4f37751421dbbe794ccf3d5a46bdf900 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);
+               skb = nskb;
+               *pskb = skb;
+       }
+       dev_hold(dev);
+       nskb = skb_clone(skb, GFP_ATOMIC);
        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;
        }

^ permalink raw reply related	[flat|nested] 6+ messages in thread

* 答复: [PATCH][next-next][v2] netlink: avoid to allocate full skb when sending to many devices
  2018-09-20 13:51   ` Eric Dumazet
@ 2018-09-21  3:27     ` Li,Rongqing
  2018-09-21 14:54       ` Eric Dumazet
  0 siblings, 1 reply; 6+ messages in thread
From: Li,Rongqing @ 2018-09-21  3:27 UTC (permalink / raw)
  To: Eric Dumazet, netdev

: 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,  

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

^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: 答复: [PATCH][next-next][v2] netlink: avoid to allocate full skb when sending to many devices
  2018-09-21  3:27     ` 答复: " Li,Rongqing
@ 2018-09-21 14:54       ` Eric Dumazet
  0 siblings, 0 replies; 6+ messages in thread
From: Eric Dumazet @ 2018-09-21 14:54 UTC (permalink / raw)
  To: Li,Rongqing, Eric Dumazet, netdev



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.

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH][next-next][v2] netlink: avoid to allocate full skb when sending to many devices
  2018-09-20  8:54 [PATCH][next-next][v2] netlink: avoid to allocate full skb when sending to many devices Li RongQing
  2018-09-20 13:43 ` Eric Dumazet
@ 2018-09-21 19:08 ` Cong Wang
  1 sibling, 0 replies; 6+ messages in thread
From: Cong Wang @ 2018-09-21 19:08 UTC (permalink / raw)
  To: Li,Rongqing; +Cc: Linux Kernel Network Developers

On Thu, Sep 20, 2018 at 1:58 AM Li RongQing <lirongqing@baidu.com> wrote:
>
> if skb->head is vmalloc address, when this skb is delivered, full
> allocation for this skb is required, if there are many devices,
> the full allocation will be called for every devices

So why do you in practice need many netlink tap devices?

tap devices are now isolated in each netns, so at max you need
one tap device per netns in practice.

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2018-09-22  0:58 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-09-20  8:54 [PATCH][next-next][v2] netlink: avoid to allocate full skb when sending to many devices Li RongQing
2018-09-20 13:43 ` Eric Dumazet
2018-09-20 13:51   ` Eric Dumazet
2018-09-21  3:27     ` 答复: " Li,Rongqing
2018-09-21 14:54       ` Eric Dumazet
2018-09-21 19:08 ` Cong Wang

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.