Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1367821 > unrolled thread
| Started by | Jason Wang <jasowang@redhat.com> |
|---|---|
| First post | 2016-03-31 08:00 +0200 |
| Last post | 2016-04-01 15:10 +0200 |
| Articles | 8 — 3 participants |
Back to article view | Back to linux.kernel
This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by
below is the oldest one visible, not the original post.
[PATCH net-next 1/6] net: skbuff: don't use union for napi_id and sender_cpu Jason Wang <jasowang@redhat.com> - 2016-03-31 08:00 +0200
Re: [PATCH net-next 1/6] net: skbuff: don't use union for napi_id and sender_cpu Eric Dumazet <eric.dumazet@gmail.com> - 2016-03-31 12:40 +0200
Re: [PATCH net-next 1/6] net: skbuff: don't use union for napi_id and sender_cpu David Miller <davem@davemloft.net> - 2016-03-31 22:10 +0200
Re: [PATCH net-next 1/6] net: skbuff: don't use union for napi_id and sender_cpu Jason Wang <jasowang@redhat.com> - 2016-04-01 04:50 +0200
Re: [PATCH net-next 1/6] net: skbuff: don't use union for napi_id and sender_cpu Jason Wang <jasowang@redhat.com> - 2016-04-01 04:20 +0200
Re: [PATCH net-next 1/6] net: skbuff: don't use union for napi_id and sender_cpu Eric Dumazet <eric.dumazet@gmail.com> - 2016-04-01 05:00 +0200
Re: [PATCH net-next 1/6] net: skbuff: don't use union for napi_id and sender_cpu Jason Wang <jasowang@redhat.com> - 2016-04-01 06:50 +0200
Re: [PATCH net-next 1/6] net: skbuff: don't use union for napi_id and sender_cpu Eric Dumazet <eric.dumazet@gmail.com> - 2016-04-01 15:10 +0200
| From | Jason Wang <jasowang@redhat.com> |
|---|---|
| Date | 2016-03-31 08:00 +0200 |
| Subject | [PATCH net-next 1/6] net: skbuff: don't use union for napi_id and sender_cpu |
| Message-ID | <riDOa-iF-7@gated-at.bofh.it> |
We use a union for napi_id and send_cpu, this is ok for most of the
cases except when we want to support busy polling for tun which needs
napi_id to be stored and passed to socket during tun_net_xmit(). In
this case, napi_id was overridden with sender_cpu before tun_net_xmit()
was called if XPS was enabled. Fixing by not using union for napi_id
and sender_cpu.
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
include/linux/skbuff.h | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index 15d0df9..8aee891 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -743,11 +743,11 @@ struct sk_buff {
__u32 hash;
__be16 vlan_proto;
__u16 vlan_tci;
-#if defined(CONFIG_NET_RX_BUSY_POLL) || defined(CONFIG_XPS)
- union {
- unsigned int napi_id;
- unsigned int sender_cpu;
- };
+#if defined(CONFIG_NET_RX_BUSY_POLL)
+ unsigned int napi_id;
+#endif
+#if defined(CONFIG_XPS)
+ unsigned int sender_cpu;
#endif
union {
#ifdef CONFIG_NETWORK_SECMARK
--
2.5.0
[toc] | [next] | [standalone]
| From | Eric Dumazet <eric.dumazet@gmail.com> |
|---|---|
| Date | 2016-03-31 12:40 +0200 |
| Subject | Re: [PATCH net-next 1/6] net: skbuff: don't use union for napi_id and sender_cpu |
| Message-ID | <riIb9-3Af-39@gated-at.bofh.it> |
| In reply to | #1367821 |
On Thu, 2016-03-31 at 13:50 +0800, Jason Wang wrote:
> We use a union for napi_id and send_cpu, this is ok for most of the
> cases except when we want to support busy polling for tun which needs
> napi_id to be stored and passed to socket during tun_net_xmit(). In
> this case, napi_id was overridden with sender_cpu before tun_net_xmit()
> was called if XPS was enabled. Fixing by not using union for napi_id
> and sender_cpu.
>
> Signed-off-by: Jason Wang <jasowang@redhat.com>
> ---
> include/linux/skbuff.h | 10 +++++-----
> 1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
> index 15d0df9..8aee891 100644
> --- a/include/linux/skbuff.h
> +++ b/include/linux/skbuff.h
> @@ -743,11 +743,11 @@ struct sk_buff {
> __u32 hash;
> __be16 vlan_proto;
> __u16 vlan_tci;
> -#if defined(CONFIG_NET_RX_BUSY_POLL) || defined(CONFIG_XPS)
> - union {
> - unsigned int napi_id;
> - unsigned int sender_cpu;
> - };
> +#if defined(CONFIG_NET_RX_BUSY_POLL)
> + unsigned int napi_id;
> +#endif
> +#if defined(CONFIG_XPS)
> + unsigned int sender_cpu;
> #endif
> union {
> #ifdef CONFIG_NETWORK_SECMARK
Hmmm...
This is a serious problem.
Making skb bigger (8 bytes because of alignment) was not considered
valid for sender_cpu introduction. We worked quite hard to avoid this,
if you take a look at git history :(
Can you describe more precisely the problem and code path ?
[toc] | [prev] | [next] | [standalone]
| From | David Miller <davem@davemloft.net> |
|---|---|
| Date | 2016-03-31 22:10 +0200 |
| Subject | Re: [PATCH net-next 1/6] net: skbuff: don't use union for napi_id and sender_cpu |
| Message-ID | <riR4K-1LT-5@gated-at.bofh.it> |
| In reply to | #1368163 |
From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Thu, 31 Mar 2016 03:32:21 -0700
> On Thu, 2016-03-31 at 13:50 +0800, Jason Wang wrote:
>> We use a union for napi_id and send_cpu, this is ok for most of the
>> cases except when we want to support busy polling for tun which needs
>> napi_id to be stored and passed to socket during tun_net_xmit(). In
>> this case, napi_id was overridden with sender_cpu before tun_net_xmit()
>> was called if XPS was enabled. Fixing by not using union for napi_id
>> and sender_cpu.
>>
>> Signed-off-by: Jason Wang <jasowang@redhat.com>
>> ---
>> include/linux/skbuff.h | 10 +++++-----
>> 1 file changed, 5 insertions(+), 5 deletions(-)
>>
>> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
>> index 15d0df9..8aee891 100644
>> --- a/include/linux/skbuff.h
>> +++ b/include/linux/skbuff.h
>> @@ -743,11 +743,11 @@ struct sk_buff {
>> __u32 hash;
>> __be16 vlan_proto;
>> __u16 vlan_tci;
>> -#if defined(CONFIG_NET_RX_BUSY_POLL) || defined(CONFIG_XPS)
>> - union {
>> - unsigned int napi_id;
>> - unsigned int sender_cpu;
>> - };
>> +#if defined(CONFIG_NET_RX_BUSY_POLL)
>> + unsigned int napi_id;
>> +#endif
>> +#if defined(CONFIG_XPS)
>> + unsigned int sender_cpu;
>> #endif
>> union {
>> #ifdef CONFIG_NETWORK_SECMARK
>
> Hmmm...
>
> This is a serious problem.
>
> Making skb bigger (8 bytes because of alignment) was not considered
> valid for sender_cpu introduction. We worked quite hard to avoid this,
> if you take a look at git history :(
>
> Can you describe more precisely the problem and code path ?
From what I can see they are doing busy poll loops in the TX code paths,
as well as the RX code paths, of vhost.
Doing this in the TX side makes little sense to me. The busy poll
implementations in the drivers only process their RX queues when
->ndo_busy_poll() is invoked. So I wonder what this is accomplishing
for the vhost TX case?
[toc] | [prev] | [next] | [standalone]
| From | Jason Wang <jasowang@redhat.com> |
|---|---|
| Date | 2016-04-01 04:50 +0200 |
| Subject | Re: [PATCH net-next 1/6] net: skbuff: don't use union for napi_id and sender_cpu |
| Message-ID | <riXjQ-6eO-7@gated-at.bofh.it> |
| In reply to | #1368585 |
On 04/01/2016 04:01 AM, David Miller wrote:
> From: Eric Dumazet <eric.dumazet@gmail.com>
> Date: Thu, 31 Mar 2016 03:32:21 -0700
>
>> On Thu, 2016-03-31 at 13:50 +0800, Jason Wang wrote:
>>> We use a union for napi_id and send_cpu, this is ok for most of the
>>> cases except when we want to support busy polling for tun which needs
>>> napi_id to be stored and passed to socket during tun_net_xmit(). In
>>> this case, napi_id was overridden with sender_cpu before tun_net_xmit()
>>> was called if XPS was enabled. Fixing by not using union for napi_id
>>> and sender_cpu.
>>>
>>> Signed-off-by: Jason Wang <jasowang@redhat.com>
>>> ---
>>> include/linux/skbuff.h | 10 +++++-----
>>> 1 file changed, 5 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
>>> index 15d0df9..8aee891 100644
>>> --- a/include/linux/skbuff.h
>>> +++ b/include/linux/skbuff.h
>>> @@ -743,11 +743,11 @@ struct sk_buff {
>>> __u32 hash;
>>> __be16 vlan_proto;
>>> __u16 vlan_tci;
>>> -#if defined(CONFIG_NET_RX_BUSY_POLL) || defined(CONFIG_XPS)
>>> - union {
>>> - unsigned int napi_id;
>>> - unsigned int sender_cpu;
>>> - };
>>> +#if defined(CONFIG_NET_RX_BUSY_POLL)
>>> + unsigned int napi_id;
>>> +#endif
>>> +#if defined(CONFIG_XPS)
>>> + unsigned int sender_cpu;
>>> #endif
>>> union {
>>> #ifdef CONFIG_NETWORK_SECMARK
>> Hmmm...
>>
>> This is a serious problem.
>>
>> Making skb bigger (8 bytes because of alignment) was not considered
>> valid for sender_cpu introduction. We worked quite hard to avoid this,
>> if you take a look at git history :(
>>
>> Can you describe more precisely the problem and code path ?
> From what I can see they are doing busy poll loops in the TX code paths,
> as well as the RX code paths, of vhost.
>
> Doing this in the TX side makes little sense to me. The busy poll
> implementations in the drivers only process their RX queues when
> ->ndo_busy_poll() is invoked. So I wonder what this is accomplishing
> for the vhost TX case?
In vhost TX case, it's possible that new packets were arrived at rx
queue during tx polling. Consider tx and rx were processed in one
thread, poll rx looks feasible to me.
[toc] | [prev] | [next] | [standalone]
| From | Jason Wang <jasowang@redhat.com> |
|---|---|
| Date | 2016-04-01 04:20 +0200 |
| Subject | Re: [PATCH net-next 1/6] net: skbuff: don't use union for napi_id and sender_cpu |
| Message-ID | <riWQO-62F-13@gated-at.bofh.it> |
| In reply to | #1368163 |
On 03/31/2016 06:32 PM, Eric Dumazet wrote:
> On Thu, 2016-03-31 at 13:50 +0800, Jason Wang wrote:
>> We use a union for napi_id and send_cpu, this is ok for most of the
>> cases except when we want to support busy polling for tun which needs
>> napi_id to be stored and passed to socket during tun_net_xmit(). In
>> this case, napi_id was overridden with sender_cpu before tun_net_xmit()
>> was called if XPS was enabled. Fixing by not using union for napi_id
>> and sender_cpu.
>>
>> Signed-off-by: Jason Wang <jasowang@redhat.com>
>> ---
>> include/linux/skbuff.h | 10 +++++-----
>> 1 file changed, 5 insertions(+), 5 deletions(-)
>>
>> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
>> index 15d0df9..8aee891 100644
>> --- a/include/linux/skbuff.h
>> +++ b/include/linux/skbuff.h
>> @@ -743,11 +743,11 @@ struct sk_buff {
>> __u32 hash;
>> __be16 vlan_proto;
>> __u16 vlan_tci;
>> -#if defined(CONFIG_NET_RX_BUSY_POLL) || defined(CONFIG_XPS)
>> - union {
>> - unsigned int napi_id;
>> - unsigned int sender_cpu;
>> - };
>> +#if defined(CONFIG_NET_RX_BUSY_POLL)
>> + unsigned int napi_id;
>> +#endif
>> +#if defined(CONFIG_XPS)
>> + unsigned int sender_cpu;
>> #endif
>> union {
>> #ifdef CONFIG_NETWORK_SECMARK
> Hmmm...
>
> This is a serious problem.
>
> Making skb bigger (8 bytes because of alignment) was not considered
> valid for sender_cpu introduction. We worked quite hard to avoid this,
> if you take a look at git history :(
>
> Can you describe more precisely the problem and code path ?
>
The problem is we want to support busy polling for tun. This needs
napi_id to be passed to tun socket by sk_mark_napi_id() during
tun_net_xmit(). But before reaching this, XPS will set sender_cpu will
make us can't see correct napi_id.
[toc] | [prev] | [next] | [standalone]
| From | Eric Dumazet <eric.dumazet@gmail.com> |
|---|---|
| Date | 2016-04-01 05:00 +0200 |
| Subject | Re: [PATCH net-next 1/6] net: skbuff: don't use union for napi_id and sender_cpu |
| Message-ID | <riXtx-6ie-9@gated-at.bofh.it> |
| In reply to | #1368961 |
On Fri, 2016-04-01 at 10:13 +0800, Jason Wang wrote: > > The problem is we want to support busy polling for tun. This needs > napi_id to be passed to tun socket by sk_mark_napi_id() during > tun_net_xmit(). But before reaching this, XPS will set sender_cpu will > make us can't see correct napi_id. > Looks like napi_id should have precedence then ? Only forwarding should allow the field to be cleared to allow XPS to do its job. Maybe skb_sender_cpu_clear() was removed too early (commit 64d4e3431e686dc37ce388ba531c4c4e866fb141) Look, it is 8pm here, I am pretty sure a solution can be found, but I am also need to take a break, I started at 3am today...
[toc] | [prev] | [next] | [standalone]
| From | Jason Wang <jasowang@redhat.com> |
|---|---|
| Date | 2016-04-01 06:50 +0200 |
| Subject | Re: [PATCH net-next 1/6] net: skbuff: don't use union for napi_id and sender_cpu |
| Message-ID | <riZbY-7Gh-5@gated-at.bofh.it> |
| In reply to | #1368977 |
On 04/01/2016 10:55 AM, Eric Dumazet wrote: > On Fri, 2016-04-01 at 10:13 +0800, Jason Wang wrote: > > >> The problem is we want to support busy polling for tun. This needs >> napi_id to be passed to tun socket by sk_mark_napi_id() during >> tun_net_xmit(). But before reaching this, XPS will set sender_cpu will >> make us can't see correct napi_id. >> > Looks like napi_id should have precedence then ? But then when busy polling is enabled, we may still hit the issue before commit 2bd82484bb4c5db1d5dc983ac7c409b2782e0154? So looks like sometimes (e.g for tun), we need both two fields. > > Only forwarding should allow the field to be cleared to allow XPS to do > its job. > > Maybe skb_sender_cpu_clear() was removed too early (commit > 64d4e3431e686dc37ce388ba531c4c4e866fb141) Not sure I get you, but this will clear napi_id too. > Look, it is 8pm here, I am pretty sure a solution can be found, > but I am also need to take a break, I started at 3am today... > > >
[toc] | [prev] | [next] | [standalone]
| From | Eric Dumazet <eric.dumazet@gmail.com> |
|---|---|
| Date | 2016-04-01 15:10 +0200 |
| Subject | Re: [PATCH net-next 1/6] net: skbuff: don't use union for napi_id and sender_cpu |
| Message-ID | <rj6ZR-4O4-37@gated-at.bofh.it> |
| In reply to | #1369000 |
On Fri, 2016-04-01 at 12:49 +0800, Jason Wang wrote: > > On 04/01/2016 10:55 AM, Eric Dumazet wrote: > > On Fri, 2016-04-01 at 10:13 +0800, Jason Wang wrote: > > > > > >> The problem is we want to support busy polling for tun. This needs > >> napi_id to be passed to tun socket by sk_mark_napi_id() during > >> tun_net_xmit(). But before reaching this, XPS will set sender_cpu will > >> make us can't see correct napi_id. > >> > > Looks like napi_id should have precedence then ? > > But then when busy polling is enabled, we may still hit the issue before > commit 2bd82484bb4c5db1d5dc983ac7c409b2782e0154? So looks like sometimes > (e.g for tun), we need both two fields. You did not clearly show me the path you take where both fields would be needed. If you expect me to do that, it wont happen. > > > > > Only forwarding should allow the field to be cleared to allow XPS to do > > its job. > > > > Maybe skb_sender_cpu_clear() was removed too early (commit > > 64d4e3431e686dc37ce388ba531c4c4e866fb141) > > Not sure I get you, but this will clear napi_id too. Only when allowed. In your case it would not be called. Some people do not use tun, and want to forward or cook millions of packets per second. sk_buff size is critical. If busy polling gives you 5 % of performance improvement, but cost everyone else a performance decrease, this is a serious problem. XPS is a sender problem, NAPI is a receiver problem. Fields should be shared.
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web