* Re: [net-next PATCH V1 1/3] net: bulk alloc and reuse of SKBs in NAPI context
From: Jesper Dangaard Brouer @ 2016-05-10 14:48 UTC (permalink / raw)
To: Eric Dumazet
Cc: Alexander Duyck, Netdev, David S. Miller, Saeed Mahameed,
Or Gerlitz, Eugenia Emantayev, brouer
In-Reply-To: <1462888134.23934.60.camel@edumazet-glaptop3.roam.corp.google.com>
On Tue, 10 May 2016 06:48:54 -0700
Eric Dumazet <eric.dumazet@gmail.com> wrote:
> On Tue, 2016-05-10 at 14:30 +0200, Jesper Dangaard Brouer wrote:
>
> > Disable busy poll on both client and server, Not patched:
> >
> > $ netperf -H 198.18.40.2 -t TCP_RR -l 60 -T 6,6 -Cc
> > MIGRATED TCP REQUEST/RESPONSE TEST from 0.0.0.0 port 0 AF_INET to 198.18.40.2
> > () port 0 AF_INET : histogram : demo : first burst 0 : cpu bind
> > Local /Remote
> > Socket Size Request Resp. Elapsed Trans. CPU CPU S.dem S.dem
> > Send Recv Size Size Time Rate local remote local remote
> > bytes bytes bytes bytes secs. per sec % S % S us/Tr us/Tr
> >
> > 16384 87380 1 1 60.00 78077.55 3.74 2.69 3.830 8.265
> > 16384 87380
>
> Tell us more about the -T6,6
>
> For example how many TX/RX queues you have on the NIC, and which cpus
> service interrupts.
The -T6,6 option:
-T lcpu,rcpu Request netperf/netserver be bound to local/remote cpu
I use the option to get more stable results. If I don't pin/bind the
CPU netperf/netserver is running on then the CPU scheduler will migrate
the processes around. This gives unpredictable results, worst for the
busy_poll tests. Especially if the RX softirq runs on the same CPU
(also true if it runs on a HyperTread siping).
Netperf client (8 cores i7-4790K CPU @ 4.00GHz) RX:8 and TX:8 queues.
Netserver server (2x 12 cores E5-2630 @ 2.30GHz) RX:8 and TX:24 queues.
Driver mlx4.
Disabled GRO to hit code path I changed in patch 2.
--
Best regards,
Jesper Dangaard Brouer
MSc.CS, Principal Kernel Engineer at Red Hat
Author of http://www.iptv-analyzer.org
LinkedIn: http://www.linkedin.com/in/brouer
^ permalink raw reply
* Re: [PATCH net-next] sfc: allocate rx pages on the same node as the interrupt
From: Eric Dumazet @ 2016-05-10 14:55 UTC (permalink / raw)
To: Edward Cree; +Cc: linux-net-drivers, David Miller, netdev, Daniel Pieczko
In-Reply-To: <5731EE51.2090908@solarflare.com>
On Tue, 2016-05-10 at 15:21 +0100, Edward Cree wrote:
> From: Daniel Pieczko <dpieczko@solarflare.com>
>
> When the interrupt servicing a channel is on a NUMA node that is
> not local to the device, performance is improved by allocating
> rx pages on the node local to the interrupt (remote to the device)
>
> The performance-optimal case, where interrupts and applications
> are pinned to CPUs on the same node as the device, is not altered
> by this change.
>
> This change gave a 1% improvement in transaction rate using Nginx
> with all interrupts and Nginx threads on the node remote to the
> device. It also gave a small reduction in round-trip latency,
> again with the interrupt and application on a different node to
> the device.
Yes, I advocated for such changes in the past for mlx4 NIC.
But your patch makes no sense to me.
alloc_pages() by default would run on the cpu servicing the IRQ, so
would automatically provide pages on the local node.
If you care only of the initial pages allocated with GFP_KERNEL at
device start, really that is a small detail as they should be consumed
and replaced quite fast.
If you worry that "wrong" pages would be reused over and over,
you could make sure that efx_reuse_page() wont reuse a page on the wrong
node.
page_to_nid(page) != numa_mem_id()
Note that this could happen even if IRQ are not changed since
alloc_pages() could in stress situations give you a page from a remote
node.
^ permalink raw reply
* Re: [PATCH net-next] sfc: allocate rx pages on the same node as the interrupt
From: Eric Dumazet @ 2016-05-10 14:57 UTC (permalink / raw)
To: Edward Cree; +Cc: linux-net-drivers, David Miller, netdev, Daniel Pieczko
In-Reply-To: <1462892106.23934.77.camel@edumazet-glaptop3.roam.corp.google.com>
On Tue, 2016-05-10 at 07:55 -0700, Eric Dumazet wrote:
> On Tue, 2016-05-10 at 15:21 +0100, Edward Cree wrote:
> > From: Daniel Pieczko <dpieczko@solarflare.com>
> >
> > When the interrupt servicing a channel is on a NUMA node that is
> > not local to the device, performance is improved by allocating
> > rx pages on the node local to the interrupt (remote to the device)
> >
> > The performance-optimal case, where interrupts and applications
> > are pinned to CPUs on the same node as the device, is not altered
> > by this change.
> >
> > This change gave a 1% improvement in transaction rate using Nginx
> > with all interrupts and Nginx threads on the node remote to the
> > device. It also gave a small reduction in round-trip latency,
> > again with the interrupt and application on a different node to
> > the device.
>
> Yes, I advocated for such changes in the past for mlx4 NIC.
>
> But your patch makes no sense to me.
>
> alloc_pages() by default would run on the cpu servicing the IRQ, so
> would automatically provide pages on the local node.
>
> If you care only of the initial pages allocated with GFP_KERNEL at
> device start, really that is a small detail as they should be consumed
> and replaced quite fast.
>
> If you worry that "wrong" pages would be reused over and over,
> you could make sure that efx_reuse_page() wont reuse a page on the wrong
> node.
>
> page_to_nid(page) != numa_mem_id()
>
> Note that this could happen even if IRQ are not changed since
> alloc_pages() could in stress situations give you a page from a remote
> node.
Something like this (untested) ?
diff --git a/drivers/net/ethernet/sfc/rx.c b/drivers/net/ethernet/sfc/rx.c
index 8956995b2fe713b377f205a55fada36c8a04253a..90665a8992e29cceb29dd465b52f934dff3b3d4e 100644
--- a/drivers/net/ethernet/sfc/rx.c
+++ b/drivers/net/ethernet/sfc/rx.c
@@ -124,7 +124,7 @@ static struct page *efx_reuse_page(struct efx_rx_queue *rx_queue)
++rx_queue->page_remove;
/* If page_count is 1 then we hold the only reference to this page. */
- if (page_count(page) == 1) {
+ if (page_count(page) == 1 && page_to_nid(page) == numa_mem_id()) {
++rx_queue->page_recycle_count;
return page;
} else {
^ permalink raw reply related
* Re: [PATCH net-next] sfc: allocate rx pages on the same node as the interrupt
From: Eric Dumazet @ 2016-05-10 14:59 UTC (permalink / raw)
To: Edward Cree; +Cc: linux-net-drivers, David Miller, netdev, Daniel Pieczko
In-Reply-To: <1462892223.23934.78.camel@edumazet-glaptop3.roam.corp.google.com>
On Tue, 2016-05-10 at 07:57 -0700, Eric Dumazet wrote:
> On Tue, 2016-05-10 at 07:55 -0700, Eric Dumazet wrote:
> > On Tue, 2016-05-10 at 15:21 +0100, Edward Cree wrote:
> > > From: Daniel Pieczko <dpieczko@solarflare.com>
> > >
> > > When the interrupt servicing a channel is on a NUMA node that is
> > > not local to the device, performance is improved by allocating
> > > rx pages on the node local to the interrupt (remote to the device)
> > >
> > > The performance-optimal case, where interrupts and applications
> > > are pinned to CPUs on the same node as the device, is not altered
> > > by this change.
> > >
> > > This change gave a 1% improvement in transaction rate using Nginx
> > > with all interrupts and Nginx threads on the node remote to the
> > > device. It also gave a small reduction in round-trip latency,
> > > again with the interrupt and application on a different node to
> > > the device.
> >
> > Yes, I advocated for such changes in the past for mlx4 NIC.
> >
> > But your patch makes no sense to me.
> >
> > alloc_pages() by default would run on the cpu servicing the IRQ, so
> > would automatically provide pages on the local node.
> >
> > If you care only of the initial pages allocated with GFP_KERNEL at
> > device start, really that is a small detail as they should be consumed
> > and replaced quite fast.
> >
> > If you worry that "wrong" pages would be reused over and over,
> > you could make sure that efx_reuse_page() wont reuse a page on the wrong
> > node.
> >
> > page_to_nid(page) != numa_mem_id()
> >
> > Note that this could happen even if IRQ are not changed since
> > alloc_pages() could in stress situations give you a page from a remote
> > node.
>
> Something like this (untested) ?
>
> diff --git a/drivers/net/ethernet/sfc/rx.c b/drivers/net/ethernet/sfc/rx.c
> index 8956995b2fe713b377f205a55fada36c8a04253a..90665a8992e29cceb29dd465b52f934dff3b3d4e 100644
> --- a/drivers/net/ethernet/sfc/rx.c
> +++ b/drivers/net/ethernet/sfc/rx.c
> @@ -124,7 +124,7 @@ static struct page *efx_reuse_page(struct efx_rx_queue *rx_queue)
> ++rx_queue->page_remove;
>
> /* If page_count is 1 then we hold the only reference to this page. */
> - if (page_count(page) == 1) {
> + if (page_count(page) == 1 && page_to_nid(page) == numa_mem_id()) {
> ++rx_queue->page_recycle_count;
> return page;
> } else {
>
You could also factorize code from Intel drivers in some common helper
static inline bool igb_page_is_reserved(struct page *page)
{
return (page_to_nid(page) != numa_mem_id()) || page_is_pfmemalloc(page);
}
^ permalink raw reply
* Re: [PATCH net] tcp: refresh skb timestamp at retransmit time
From: Yuchung Cheng @ 2016-05-10 15:01 UTC (permalink / raw)
To: Eric Dumazet; +Cc: David Miller, netdev
In-Reply-To: <1462852516.23934.46.camel@edumazet-glaptop3.roam.corp.google.com>
On Mon, May 9, 2016 at 8:55 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> From: Eric Dumazet <edumazet@google.com>
>
> In the very unlikely case __tcp_retransmit_skb() can not use the cloning
> done in tcp_transmit_skb(), we need to refresh skb_mstamp before doing
> the copy and transmit, otherwise TCP TS val will be an exact copy of
> original transmit.
>
> Fixes: 7faee5c0d514 ("tcp: remove TCP_SKB_CB(skb)->when")
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Cc: Yuchung Cheng <ycheng@google.com>
Acked-by: Yuchung Cheng <ycheng@google.com>
Nice catch Eric. Recovery algorithm like RACK definitely requires this
patch b/c it relies on skb mstamps.
does the failure usually occur under memory stress?
> ---
> net/ipv4/tcp_output.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
> index 441ae9da3a23..79a03b87a771 100644
> --- a/net/ipv4/tcp_output.c
> +++ b/net/ipv4/tcp_output.c
> @@ -2640,8 +2640,10 @@ int __tcp_retransmit_skb(struct sock *sk, struct sk_buff *skb)
> */
> if (unlikely((NET_IP_ALIGN && ((unsigned long)skb->data & 3)) ||
> skb_headroom(skb) >= 0xFFFF)) {
> - struct sk_buff *nskb = __pskb_copy(skb, MAX_TCP_HEADER,
> - GFP_ATOMIC);
> + struct sk_buff *nskb;
> +
> + skb_mstamp_get(&skb->skb_mstamp);
> + nskb = __pskb_copy(skb, MAX_TCP_HEADER, GFP_ATOMIC);
> err = nskb ? tcp_transmit_skb(sk, nskb, 0, GFP_ATOMIC) :
> -ENOBUFS;
> } else {
>
>
^ permalink raw reply
* Re: [Drbd-dev] [PATCH net-next v3] block/drbd: align properly u64 in nl messages
From: David Miller @ 2016-05-10 15:39 UTC (permalink / raw)
To: lars.ellenberg
Cc: nicolas.dichtel, netdev, philipp.reisner, drbd-dev, linux-kernel
In-Reply-To: <20160510094023.GC16459@soda.linbit>
From: Lars Ellenberg <lars.ellenberg@linbit.com>
Date: Tue, 10 May 2016 11:40:23 +0200
> If we introduce a new config option,
> we have to add it to the config scanner (one line),
> define min, max, default and scale (four short defines),
> and add it to the netlink definition here (one line).
> Done, rest of the code is generated,
> both on the kernel side,
> and on the drbd-utils side used to talk to the kernel.
> We found that to be very convenient.
But it entirely misses the core design point of netlink.
Sender and receive _DO NOT_ need to coordinate at all. That's the
whole point. So tightly coupling such coordination is going to run
you into all kinds of problems.
When implemented properly, the sender can emit whatever attributes it
knows about and can generate, and the receive scans the attributes one
by one and picks out the ones it understands and processes them.
If you go against this model then you have no clean way to extend
things whilst allowing existing software to continue working.
If the drbd stuff had been posting to the networking list, we really
would have screamed loudly about these auto-generates structs and
macros and whatnot.
Anyways, back to the topic, can you please just relent and come to some
kind of agreement about the fix for this alignment bug? This is taking
a very long time and patches are just rotting in patchwork with no
resolution.
Thanks.
^ permalink raw reply
* Re: [net-next PATCH V1 1/3] net: bulk alloc and reuse of SKBs in NAPI context
From: Eric Dumazet @ 2016-05-10 15:44 UTC (permalink / raw)
To: Jesper Dangaard Brouer
Cc: Alexander Duyck, Netdev, David S. Miller, Saeed Mahameed,
Or Gerlitz, Eugenia Emantayev
In-Reply-To: <20160510164857.72c8a1cb@redhat.com>
On Tue, 2016-05-10 at 16:48 +0200, Jesper Dangaard Brouer wrote:
> On Tue, 10 May 2016 06:48:54 -0700
> Eric Dumazet <eric.dumazet@gmail.com> wrote:
>
> > On Tue, 2016-05-10 at 14:30 +0200, Jesper Dangaard Brouer wrote:
> >
> > > Disable busy poll on both client and server, Not patched:
> > >
> > > $ netperf -H 198.18.40.2 -t TCP_RR -l 60 -T 6,6 -Cc
> > > MIGRATED TCP REQUEST/RESPONSE TEST from 0.0.0.0 port 0 AF_INET to 198.18.40.2
> > > () port 0 AF_INET : histogram : demo : first burst 0 : cpu bind
> > > Local /Remote
> > > Socket Size Request Resp. Elapsed Trans. CPU CPU S.dem S.dem
> > > Send Recv Size Size Time Rate local remote local remote
> > > bytes bytes bytes bytes secs. per sec % S % S us/Tr us/Tr
> > >
> > > 16384 87380 1 1 60.00 78077.55 3.74 2.69 3.830 8.265
> > > 16384 87380
> >
> > Tell us more about the -T6,6
> >
> > For example how many TX/RX queues you have on the NIC, and which cpus
> > service interrupts.
>
> The -T6,6 option:
> -T lcpu,rcpu Request netperf/netserver be bound to local/remote cpu
>
Sure, I know -T option in netperf.
> I use the option to get more stable results. If I don't pin/bind the
> CPU netperf/netserver is running on then the CPU scheduler will migrate
> the processes around. This gives unpredictable results, worst for the
> busy_poll tests. Especially if the RX softirq runs on the same CPU
> (also true if it runs on a HyperTread siping).
>
> Netperf client (8 cores i7-4790K CPU @ 4.00GHz) RX:8 and TX:8 queues.
> Netserver server (2x 12 cores E5-2630 @ 2.30GHz) RX:8 and TX:24 queues.
> Driver mlx4.
> Disabled GRO to hit code path I changed in patch 2.
But are you using stuff like aRFS, RPS , RFS ?
Each netperf run lands on different cpus, and we know that results can
have a 25% variability because of that, even more on 2-node systems.
By forcing -T6,6 you force the netperf/netserver cpu, not the RX queues.
A nice effort would be to be able to chose the source in the 4-tuple at
connect() time so that we know that Toeplitz hash will select the
'correct' RX queue.
^ permalink raw reply
* Re: [RFC PATCH 0/2] net: threadable napi poll loop
From: David Miller @ 2016-05-10 15:51 UTC (permalink / raw)
To: eric.dumazet
Cc: pabeni, netdev, edumazet, jiri, daniel, ast, aduyck, tom, peterz,
mingo, riel, hannes, linux-kernel
In-Reply-To: <1462890590.23934.68.camel@edumazet-glaptop3.roam.corp.google.com>
From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Tue, 10 May 2016 07:29:50 -0700
> We already have ksoftirqd to normally cope with the case you are
> describing.
>
> If it is not working as intended, please identify the bugs and fix them,
> instead of adding yet another tests in fast path and extra complexity in
> the stack.
+1
Indeed, if ksoftirqd is not doing it's job, please fix it. It is designed
exactly to deal with the problems described here.
^ permalink raw reply
* Re: [PATCH net] tcp: refresh skb timestamp at retransmit time
From: Eric Dumazet @ 2016-05-10 15:51 UTC (permalink / raw)
To: Yuchung Cheng; +Cc: David Miller, netdev
In-Reply-To: <CAK6E8=eGaBL9O55xCbtoZuuisvUGF5NRv6qV5bbvQB-JFw6Zjg@mail.gmail.com>
On Tue, 2016-05-10 at 08:01 -0700, Yuchung Cheng wrote:
> On Mon, May 9, 2016 at 8:55 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> > From: Eric Dumazet <edumazet@google.com>
> >
> > In the very unlikely case __tcp_retransmit_skb() can not use the cloning
> > done in tcp_transmit_skb(), we need to refresh skb_mstamp before doing
> > the copy and transmit, otherwise TCP TS val will be an exact copy of
> > original transmit.
> >
> > Fixes: 7faee5c0d514 ("tcp: remove TCP_SKB_CB(skb)->when")
> > Signed-off-by: Eric Dumazet <edumazet@google.com>
> > Cc: Yuchung Cheng <ycheng@google.com>
> Acked-by: Yuchung Cheng <ycheng@google.com>
>
> Nice catch Eric. Recovery algorithm like RACK definitely requires this
> patch b/c it relies on skb mstamps.
> does the failure usually occur under memory stress?
For x86, the NET_IP_ALIGN is 0, so the only 'problem' would happen
for devices with big MTU but no SG support.
In the normal case, we allocate small skb->head skbs
(SKB_WITH_OVERHEAD(2048 - MAX_TCP_HEADER) in select_size()
So this bug should not happen for most devices.
RACK will be better, but I was also wondering if PAWS checks on receiver
could drop all subsequent retransmits and we would have a TCP stalled
connection ? That would be a more serious bug.
For arches with NET_IP_ALIGN==2, the bug would be possible if the
receiver is playing games by partially acking the packets we send.
^ permalink raw reply
* Re: [PATCH net-next v2] ila: ipv6/ila: fix nlsize calculation for lwtunnel
From: Tom Herbert @ 2016-05-10 15:51 UTC (permalink / raw)
To: Nicolas Dichtel; +Cc: Linux Kernel Network Developers, David S. Miller
In-Reply-To: <1462874192-8360-1-git-send-email-nicolas.dichtel@6wind.com>
On Tue, May 10, 2016 at 2:56 AM, Nicolas Dichtel
<nicolas.dichtel@6wind.com> wrote:
> From: Tom Herbert <tom@herbertland.com>
>
> The handler 'ila_fill_encap_info' adds two attributes: ILA_ATTR_LOCATOR
> and ILA_ATTR_CSUM_MODE.
>
> nla_total_size_64bit() must be use for ILA_ATTR_LOCATOR.
>
> Also, do nla_put_u8 instead of nla_put_u64 for ILA_ATTR_CSUM_MODE.
>
> Fixes: f13a82d87b21 ("ipv6: use nla_put_u64_64bit()")
> Fixes: 90bfe662db13 ("ila: add checksum neutral ILA translations")
> Reported-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
> Signed-off-by: Tom Herbert <tom@herbertland.com>
> Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
> ---
>
> Tom, I have taken the liberty of resending your patch, I hope it's ok
> for you. Goal is to fix this before net-next closes.
>
> v1 -> v2:
> update the patch after the merge of net (and thus update 'Fixes' tag)
> use nla_total_size_64bit() for ILA_ATTR_LOCATOR
>
> net/ipv6/ila/ila_lwt.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/net/ipv6/ila/ila_lwt.c b/net/ipv6/ila/ila_lwt.c
> index 17038e1ede98..1dfb64166d7d 100644
> --- a/net/ipv6/ila/ila_lwt.c
> +++ b/net/ipv6/ila/ila_lwt.c
> @@ -133,7 +133,7 @@ static int ila_fill_encap_info(struct sk_buff *skb,
> if (nla_put_u64_64bit(skb, ILA_ATTR_LOCATOR, (__force u64)p->locator.v64,
> ILA_ATTR_PAD))
> goto nla_put_failure;
> - if (nla_put_u64(skb, ILA_ATTR_CSUM_MODE, (__force u8)p->csum_mode))
> + if (nla_put_u8(skb, ILA_ATTR_CSUM_MODE, (__force u8)p->csum_mode))
> goto nla_put_failure;
>
> return 0;
> @@ -144,7 +144,9 @@ nla_put_failure:
>
> static int ila_encap_nlsize(struct lwtunnel_state *lwtstate)
> {
> - return nla_total_size(sizeof(u64)); /* ILA_ATTR_LOCATOR */
> + return nla_total_size_64bit(sizeof(u64)) + /* ILA_ATTR_LOCATOR */
> + nla_total_size(sizeof(u8)) + /* ILA_ATTR_CSUM_MODE */
> + 0;
> }
>
> static int ila_encap_cmp(struct lwtunnel_state *a, struct lwtunnel_state *b)
> --
> 2.8.1
>
Acked-by: Tom Herbert <tom@herbertland.com>
^ permalink raw reply
* Re: [RFC PATCH 0/2] net: threadable napi poll loop
From: Thomas Gleixner @ 2016-05-10 15:57 UTC (permalink / raw)
To: Paolo Abeni
Cc: netdev, David S. Miller, Eric Dumazet, Jiri Pirko,
Daniel Borkmann, Alexei Starovoitov, Alexander Duyck, Tom Herbert,
Peter Zijlstra, Ingo Molnar, Rik van Riel, Hannes Frederic Sowa,
linux-kernel
In-Reply-To: <cover.1462886866.git.pabeni@redhat.com>
On Tue, 10 May 2016, Paolo Abeni wrote:
Nice patch set and very promising results!
> At this point we are not really sure if we should go with this simpler
> approach by putting NAPI itself into kthreads or leverage the threadirqs
> function by putting the whole interrupt into a thread and signaling NAPI
> that it does not reschedule itself in a softirq but to simply run at
> this particular context of the interrupt handler.
>
> While the threaded irq way seems to better integrate into the kernel and
> also other devices could move their interrupts into the threads easily
> on a common policy, we don't know how to really express the necessary
> knobs with the current device driver model (module parameters, sysfs
> attributes, etc.). This is where we would like to hear some opinions.
> NAPI would e.g. have to query the kernel if the particular IRQ/MSI if it
> should be scheduled in a softirq or in a thread, so we don't have to
> rewrite all device drivers. This might even be needed on a per rx-queue
> granularity.
Utilizing threaded irqs should be halfways simple even without touching the
device driver at all.
We can do the switch to threading in two ways:
1) Let the driver request the interrupt(s) as it does now and then have a
/proc/irq/NNN/threaded file which converts it to a threaded interrupt on
the fly. That should be fairly trivial.
2) Let the driver request the interrupt(s) as it does now and retrieve the
interrupt number which belongs to the device/queue from the network core
and let the irq core switch it over to threaded.
So the interrupt flow of the device would be:
interrupt
IRQ_WAKE_THREAD
irq thread()
{
local_bh_disable();
action->thread_fn(action->irq, action->dev_id); <-- driver handler
irq_finalize_oneshot(desc, action);
local_bh_enable();
}
The driver irq handler calls napi_schedule(). So if your napi_struct is
flagged POLL_IRQ_THREAD then you can call your polling machinery from there
instead of raising the softirq.
You surely need some way to figure out whether the interrupt is threaded when
you set up the device in order to flag your napi struct, but that should be
not too hard to achieve.
Thanks,
tglx
^ permalink raw reply
* Re: [PATCH V3 2/4] net-next: mediatek: fix gigabit and flow control advertisement
From: David Miller @ 2016-05-10 16:00 UTC (permalink / raw)
To: john; +Cc: nbd, netdev, linux-mediatek, linux-kernel
In-Reply-To: <1462856921-27441-3-git-send-email-john@phrozen.org>
From: John Crispin <john@phrozen.org>
Date: Tue, 10 May 2016 07:08:39 +0200
> @@ -236,7 +247,8 @@ static int mtk_phy_connect(struct mtk_mac *mac)
> mac->phy_dev->autoneg = AUTONEG_ENABLE;
> mac->phy_dev->speed = 0;
> mac->phy_dev->duplex = 0;
> - mac->phy_dev->supported &= PHY_BASIC_FEATURES;
> + mac->phy_dev->supported &= PHY_GBIT_FEATURES | SUPPORTED_Pause |
> + ~SUPPORTED_Asym_Pause;
All this does is clear SUPPORTED_Asym_Pause. Putting the other values
there, is therefore, superfluous.
I think you need to rethink through what you expect this expression to
accomplish.
Thanks.
^ permalink raw reply
* Re: [RFC PATCH 0/2] net: threadable napi poll loop
From: Paolo Abeni @ 2016-05-10 16:03 UTC (permalink / raw)
To: Eric Dumazet
Cc: netdev, David S. Miller, Eric Dumazet, Jiri Pirko,
Daniel Borkmann, Alexei Starovoitov, Alexander Duyck, Tom Herbert,
Peter Zijlstra, Ingo Molnar, Rik van Riel, Hannes Frederic Sowa,
linux-kernel
In-Reply-To: <1462890590.23934.68.camel@edumazet-glaptop3.roam.corp.google.com>
Hi,
On Tue, 2016-05-10 at 07:29 -0700, Eric Dumazet wrote:
> On Tue, 2016-05-10 at 16:11 +0200, Paolo Abeni wrote:
> > Currently, the softirq loop can be scheduled both inside the ksofirqd kernel
> > thread and inside any running process. This makes nearly impossible for the
> > process scheduler to balance in a fair way the amount of time that
> > a given core spends performing the softirq loop.
> >
> > Under high network load, the softirq loop can take nearly 100% of a given CPU,
> > leaving very little time for use space processing. On single core hosts, this
> > means that the user space can nearly starve; for example super_netperf
> > UDP_STREAM tests towards a remote single core vCPU guest[1] can measure an
> > aggregated throughput of a few thousands pps, and the same behavior can be
> > reproduced even on bare-metal, eventually simulating a single core with taskset
> > and/or sysfs configuration.
>
> I hate these patches and ideas guys, sorry. That is before my breakfast,
> but still...
I'm sorry, I did not meant to spoil your breakfast ;-)
> I have enough hard time dealing with loads where ksoftirqd has to
> compete with user threads that thought that playing with priorities was
> a nice idea.
I fear there is a misunderstanding. I'm not suggesting to fiddle with
priorities; the above 'taskset' reference was just an hint to replicate
the starvation issue on bare-metal in the lack of a single core host.
>
> Guess what, when they lose networking they complain.
>
> We already have ksoftirqd to normally cope with the case you are
> describing.
>
> If it is not working as intended, please identify the bugs and fix them,
> instead of adding yet another tests in fast path and extra complexity in
> the stack.
The idea it exactly that: the problem is how the softirq loop is
scheduled and executed, i.e. the current ksoftirqd/"inline loop" model.
If a single core host is under network flood, i.e. ksoftirqd is
scheduled and it eventually (after processing ~640 packets) will let the
user space process run. The latter will execute a syscall to receive a
packet, which will have to disable/enable bh at least once and that will
cause the processing of another ~640 packets. To receive a single packet
in user space, the kernel has to process more than one thousand packets.
AFAICS it can't be solved without changing how the net_rx_action is
served.
The user space starvation issue don't affect large server, but AFAIK
many small devices have a lot of out-of-tree hacks to cope with this
sort of issues.
In the VM scenario, the starvation issue was not a real concern up to a
little time ago because the vhost/tun device was not able to push
packets fast enough into the guest to trigger the issue. Recent
improvements have changed the situation.
Also, the scheduler's ability to migrate the napi threads is quite
beneficial for hypervisor when the VMs are receiving a lot of network
traffic.
Please have a look at the performance numbers.
The current patch adds a single, simple, test per napi_schedule
invocation, and with minimal changes, the kernel won't access any
additional cache-line when the napi thread is disabled. Even in the
current form, in my tests no regression is seen with the patched kernel
when the napi thread mode is disabled.
> In the one vcpu case, allowing the user thread to consume more UDP
> packets from the target UDP socket will also make your NIC drop more
> packets, that are not necessarily packets for the same socket.
That is true. But the threaded napi will not starve, i.e. the forwarding
process, to a nearly zero packet rate, while with the current code the
reverse scenario can happen.
Cheers,
Paolo
>
> So you are shifting the attack to a different target,
> at the expense of more kernel bloat.
>
>
>
^ permalink raw reply
* Re: [RFC PATCH 0/2] net: threadable napi poll loop
From: Eric Dumazet @ 2016-05-10 16:08 UTC (permalink / raw)
To: Paolo Abeni
Cc: netdev, David S. Miller, Eric Dumazet, Jiri Pirko,
Daniel Borkmann, Alexei Starovoitov, Alexander Duyck, Tom Herbert,
Peter Zijlstra, Ingo Molnar, Rik van Riel, Hannes Frederic Sowa,
linux-kernel
In-Reply-To: <1462896230.6687.74.camel@redhat.com>
On Tue, 2016-05-10 at 18:03 +0200, Paolo Abeni wrote:
> If a single core host is under network flood, i.e. ksoftirqd is
> scheduled and it eventually (after processing ~640 packets) will let the
> user space process run. The latter will execute a syscall to receive a
> packet, which will have to disable/enable bh at least once and that will
> cause the processing of another ~640 packets. To receive a single packet
> in user space, the kernel has to process more than one thousand packets.
Looks you found the bug then. Have you tried to fix it ?
^ permalink raw reply
* Re: [PATCH iproute2 net-next] ifstat: move to new RTM_GETSTATS api
From: Roopa Prabhu @ 2016-05-10 16:25 UTC (permalink / raw)
To: Jamal Hadi Salim
Cc: stephen, netdev, davem, edumazet, tgraf, nicolas.dichtel, nikolay
In-Reply-To: <573084AC.6030407@mojatatu.com>
On 5/9/16, 5:38 AM, Jamal Hadi Salim wrote:
> On 16-05-09 12:49 AM, Roopa Prabhu wrote:
>> On 4/30/16, 8:15 AM, Roopa Prabhu wrote:
>>> On 4/30/16, 3:21 AM, Jamal Hadi Salim wrote:
>
>> AFAICS ifstat history file handling today assumes all 32 bit stats.
>
> Indeed it does.
>
>> And to preserve backward compatibility, new ifstat should work with old and
>> new history files with 32bit and 64 bit stats.
>
> True.
> It may be ok to just provide a conversion tool maybe for taking 32b
> history into 64b? I dont know if someone is going to "migrate" their
> history files so even that may not be worth it.
>
>> The file format cannot be changed because of the same backward compat issues.
>> So, I am leaning towards a new history file with a new option (maybe ifstat -64) to
>> save/query 64 bit stats using the new api when available.
>>
>> I see some previous brief discussions on moving ifstat to 64 bit.
>>
>> The other option is to only change 'ip -s link show' to use the new stats api.
>>
>> let me know if there are other thoughts.
>>
>
> Is it not possible to convert to 64b - and IFLA_STAT
> becomes available just store it still in 64b?
> i.e 32b will fit in 64b space.
The only problem is again if the last saved history file had 32 bit
and 32bit counters may roll over earlier.
One more thing i am looking at right now which might work is using
the existing history file and use the info_source (aka header) in ifstat history file to indicate
32bit or 64bit stats. For 32 bit it is currently "#kernel".
For 64 bit i plan to use "#kernel64".
If we are starting a new history file, i will always use 64bit and when using an existing history file,
it will use 32bit.
And 64bit stats will be queried with RTM_GETSTATS if available or will use IFLA_STATS64 when
RTM_NEWLINK Is used.
will see if i can get this working for all cases,
thanks,
Roopa
^ permalink raw reply
* Re: [PATCH nf-next,v2] gtp: add initial driver for datapath of GPRS Tunneling Protocol (GTP-U)
From: David Miller @ 2016-05-10 16:27 UTC (permalink / raw)
To: pablo; +Cc: netdev, laforge, aschultz, openbsc
In-Reply-To: <1462748148-17764-1-git-send-email-pablo@netfilter.org>
From: Pablo Neira Ayuso <pablo@netfilter.org>
Date: Mon, 9 May 2016 00:55:48 +0200
> This is an initial implementation of a netdev driver for GTP datapath
> (GTP-U) v0 and v1, according to the GSM TS 09.60 and 3GPP TS 29.060
> standards. This tunneling protocol is used to prevent subscribers from
> accessing mobile carrier core network infrastructure.
>
> This implementation requires a GGSN userspace daemon that implements the
> signaling protocol (GTP-C), such as OpenGGSN [1]. This userspace daemon
> updates the PDP context database that represents active subscriber
> sessions through a genetlink interface.
>
> For more context on this tunneling protocol, you can check the slides
> that were presented during the NetDev 1.1 [2].
>
> Only IPv4 is supported at this time.
>
> [1] http://git.osmocom.org/openggsn/
> [2] http://www.netdevconf.org/1.1/proceedings/slides/schultz-welte-osmocom-gtp.pdf
>
> Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
> ---
> v2: Rebase on top of current HEAD to resolve minor conflict with recent PPP
> updates in include/uapi/linux/if_link.h so this applies cleanly.
Applied, thanks.
^ permalink raw reply
* [PATCH net-next] skbuff: remove unused variable `doff'
From: Sowmini Varadhan @ 2016-05-10 16:38 UTC (permalink / raw)
To: netdev; +Cc: davem, daniel, sowmini.varadhan
There are two instances of an unused variable, `doff' added by
commit 6fa01ccd8830 ("skbuff: Add pskb_extract() helper function")
in pskb_carve_inside_header() and pskb_carve_inside_nonlinear().
Remove these instances, they are not used.
Reported by: Daniel Borkmann <daniel@iogearbox.net>
Signed-off-by: Sowmini Varadhan <sowmini.varadhan@oracle.com>
---
net/core/skbuff.c | 6 ------
1 files changed, 0 insertions(+), 6 deletions(-)
diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 5586be9..f2b77e5 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -4634,7 +4634,6 @@ static int pskb_carve_inside_header(struct sk_buff *skb, const u32 off,
int size = skb_end_offset(skb);
int new_hlen = headlen - off;
u8 *data;
- int doff = 0;
size = SKB_DATA_ALIGN(size);
@@ -4674,13 +4673,11 @@ static int pskb_carve_inside_header(struct sk_buff *skb, const u32 off,
skb_free_head(skb);
}
- doff = (data - skb->head);
skb->head = data;
skb->data = data;
skb->head_frag = 0;
#ifdef NET_SKBUFF_DATA_USES_OFFSET
skb->end = size;
- doff = 0;
#else
skb->end = skb->head + size;
#endif
@@ -4761,7 +4758,6 @@ static int pskb_carve_inside_nonlinear(struct sk_buff *skb, const u32 off,
u8 *data;
const int nfrags = skb_shinfo(skb)->nr_frags;
struct skb_shared_info *shinfo;
- int doff = 0;
size = SKB_DATA_ALIGN(size);
@@ -4816,13 +4812,11 @@ static int pskb_carve_inside_nonlinear(struct sk_buff *skb, const u32 off,
}
skb_release_data(skb);
- doff = (data - skb->head);
skb->head = data;
skb->head_frag = 0;
skb->data = data;
#ifdef NET_SKBUFF_DATA_USES_OFFSET
skb->end = size;
- doff = 0;
#else
skb->end = skb->head + size;
#endif
--
1.7.1
^ permalink raw reply related
* [PATCH net] drivers: net: Don't print unpopulated net_device name
From: Harvey Hunt @ 2016-05-10 16:43 UTC (permalink / raw)
To: davem
Cc: Harvey Hunt, Robert Jarzmik, Barry Song, Marcel Ziswiler, netdev,
linux-kernel
For ethernet devices, net_device.name will be eth%d before
register_netdev() is called. Don't print the net_device name until
the format string is replaced.
Cc: Robert Jarzmik <robert.jarzmik@free.fr>
Cc: Barry Song <Baohua.Song@csr.com>
Cc: Marcel Ziswiler <marcel@ziswiler.com>
Cc: netdev@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Harvey Hunt <harvey.hunt@imgtec.com>
---
I've tested this patch on a board that has a DM9000, but haven't
tested the other two network devices.
drivers/net/ethernet/davicom/dm9000.c | 3 +--
drivers/net/ethernet/micrel/ks8695net.c | 3 +--
drivers/net/ethernet/netx-eth.c | 3 +--
3 files changed, 3 insertions(+), 6 deletions(-)
diff --git a/drivers/net/ethernet/davicom/dm9000.c b/drivers/net/ethernet/davicom/dm9000.c
index 48d9194..89dcaa1 100644
--- a/drivers/net/ethernet/davicom/dm9000.c
+++ b/drivers/net/ethernet/davicom/dm9000.c
@@ -1686,8 +1686,7 @@ dm9000_probe(struct platform_device *pdev)
}
if (!is_valid_ether_addr(ndev->dev_addr)) {
- dev_warn(db->dev, "%s: Invalid ethernet MAC address. Please "
- "set using ifconfig\n", ndev->name);
+ dev_warn(db->dev, "Invalid ethernet MAC address. Please set using ifconfig\n");
eth_hw_addr_random(ndev);
mac_src = "random";
diff --git a/drivers/net/ethernet/micrel/ks8695net.c b/drivers/net/ethernet/micrel/ks8695net.c
index a8522d8..2e2ea61 100644
--- a/drivers/net/ethernet/micrel/ks8695net.c
+++ b/drivers/net/ethernet/micrel/ks8695net.c
@@ -1456,8 +1456,7 @@ ks8695_probe(struct platform_device *pdev)
ndev->dev_addr[5] = maclow & 0xFF;
if (!is_valid_ether_addr(ndev->dev_addr))
- dev_warn(ksp->dev, "%s: Invalid ethernet MAC address. Please "
- "set using ifconfig\n", ndev->name);
+ dev_warn(ksp->dev, "Invalid ethernet MAC address. Please set using ifconfig\n");
/* In order to be efficient memory-wise, we allocate both
* rings in one go.
diff --git a/drivers/net/ethernet/netx-eth.c b/drivers/net/ethernet/netx-eth.c
index 9fbc302..c2e2151 100644
--- a/drivers/net/ethernet/netx-eth.c
+++ b/drivers/net/ethernet/netx-eth.c
@@ -358,8 +358,7 @@ static int netx_eth_enable(struct net_device *ndev)
xc_start(priv->xc);
if (!is_valid_ether_addr(ndev->dev_addr))
- printk("%s: Invalid ethernet MAC address. Please "
- "set using ifconfig\n", ndev->name);
+ printk("Invalid ethernet MAC address. Please set using ifconfig\n");
for (i=2; i<=18; i++)
pfifo_push(EMPTY_PTR_FIFO(priv->id),
--
2.8.0
^ permalink raw reply related
* RE: [PATCH net-next 01/14] qed: Add CONFIG_QED_SRIOV
From: Yuval Mintz @ 2016-05-10 17:16 UTC (permalink / raw)
To: David Miller; +Cc: netdev, Ariel Elior
In-Reply-To: <20160509.151429.403288621356878412.davem@davemloft.net>
> From: Yuval Mintz <Yuval.Mintz@qlogic.com>
> Date: Mon, 9 May 2016 16:19:10 +0300
>
> > + /* bitmap indicating which fields hold valid values */
> > + aligned_u64 valid_bitmap;
>
> There is absolutely no reason to use aligned_u64 here. That type is for handling
> a specific issue in user facing APIs, which this is not.
I'm not entirely convinced this is true; If we'll not enforce the alignment
of this 64-bit field, it's possible there will be differences between 32-bit
and 64-bit machines versions of this struct.
You have to recall that this is going to be copied via DMA between PF and VF,
so they must have the exact same representation of the structure.
[If I'm wrong on the technical part here, please correct me; I vaguely
seem to recall that this was already discussed on bnx2x's implementation
of the hw-channel which also uses aligned u64 fields]
^ permalink raw reply
* Re: [PATCH nf-next,v2] gtp: add initial driver for datapath of GPRS Tunneling Protocol (GTP-U)
From: Tom Herbert @ 2016-05-10 17:16 UTC (permalink / raw)
To: David Miller
Cc: Pablo Neira Ayuso, Linux Kernel Network Developers, laforge,
aschultz, openbsc
In-Reply-To: <20160510.122735.2275935466949423395.davem@davemloft.net>
On Tue, May 10, 2016 at 9:27 AM, David Miller <davem@davemloft.net> wrote:
> From: Pablo Neira Ayuso <pablo@netfilter.org>
> Date: Mon, 9 May 2016 00:55:48 +0200
>
>> This is an initial implementation of a netdev driver for GTP datapath
>> (GTP-U) v0 and v1, according to the GSM TS 09.60 and 3GPP TS 29.060
>> standards. This tunneling protocol is used to prevent subscribers from
>> accessing mobile carrier core network infrastructure.
>>
>> This implementation requires a GGSN userspace daemon that implements the
>> signaling protocol (GTP-C), such as OpenGGSN [1]. This userspace daemon
>> updates the PDP context database that represents active subscriber
>> sessions through a genetlink interface.
>>
>> For more context on this tunneling protocol, you can check the slides
>> that were presented during the NetDev 1.1 [2].
>>
>> Only IPv4 is supported at this time.
>>
Is there a timeline for adding IPv6 support?
Tom
>> [1] http://git.osmocom.org/openggsn/
>> [2] http://www.netdevconf.org/1.1/proceedings/slides/schultz-welte-osmocom-gtp.pdf
>>
>> Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
>> ---
>> v2: Rebase on top of current HEAD to resolve minor conflict with recent PPP
>> updates in include/uapi/linux/if_link.h so this applies cleanly.
>
> Applied, thanks.
^ permalink raw reply
* Re: [PATCH nf-next,v2] gtp: add initial driver for datapath of GPRS Tunneling Protocol (GTP-U)
From: Pablo Neira Ayuso @ 2016-05-10 17:31 UTC (permalink / raw)
To: Tom Herbert
Cc: David Miller, Linux Kernel Network Developers, laforge, aschultz,
openbsc
In-Reply-To: <CALx6S34cHSNmC-QLO8QrbktTwvJMpjkOnsQLEq3W4FLBannBRg@mail.gmail.com>
On Tue, May 10, 2016 at 10:16:33AM -0700, Tom Herbert wrote:
> On Tue, May 10, 2016 at 9:27 AM, David Miller <davem@davemloft.net> wrote:
> > From: Pablo Neira Ayuso <pablo@netfilter.org>
> > Date: Mon, 9 May 2016 00:55:48 +0200
> >
> >> This is an initial implementation of a netdev driver for GTP datapath
> >> (GTP-U) v0 and v1, according to the GSM TS 09.60 and 3GPP TS 29.060
> >> standards. This tunneling protocol is used to prevent subscribers from
> >> accessing mobile carrier core network infrastructure.
> >>
> >> This implementation requires a GGSN userspace daemon that implements the
> >> signaling protocol (GTP-C), such as OpenGGSN [1]. This userspace daemon
> >> updates the PDP context database that represents active subscriber
> >> sessions through a genetlink interface.
> >>
> >> For more context on this tunneling protocol, you can check the slides
> >> that were presented during the NetDev 1.1 [2].
> >>
> >> Only IPv4 is supported at this time.
> >>
>
> Is there a timeline for adding IPv6 support?
There is a preliminary kernel patch to add IPv6 that seems to be
untested yet, I can share it with you or anyone else want to have a
look. Specifically, there are missing bits on the netlink side of the
PDP context database to support SGSN and MS IPv6 address, but that
shouldn't be much of a problem.
On the userspace side, the userspace daemon OpenGGSN still doesn't
support IPv6. Adding IPv6 support to this daemon is a bit of a PITA,
several people using it in production told me that the daemon runs
stable for production, but from a developer perspective the current
codebase look not easy to extend (quite many stuff very IPv4 specific,
I already spend time trying to refactor it two years ago to prepare
this support). Meanwhile, this triggered a new daemon daemon
implementation osmo-ggsn to replace it, I posted a proof-of-concept on
the openbsc mailing list, but nobody jumped on this to support this
development effort so far.
^ permalink raw reply
* Re: [net-next PATCH V1 1/3] net: bulk alloc and reuse of SKBs in NAPI context
From: Alexander Duyck @ 2016-05-10 17:46 UTC (permalink / raw)
To: Jesper Dangaard Brouer
Cc: Netdev, David S. Miller, Saeed Mahameed, Or Gerlitz,
Eugenia Emantayev
In-Reply-To: <20160510143017.212c3846@redhat.com>
On 05/10/2016 05:30 AM, Jesper Dangaard Brouer wrote:
>
> On Mon, 9 May 2016 13:46:32 -0700
> Alexander Duyck <alexander.duyck@gmail.com> wrote:
>
>> Try testing with TCP_RR instead and watch the CPU utilization. I'm
>> suspecting allocating 8 and freeing 7 buffers for every 1 buffer
>> received will blow any gains right out of the water. Also try it with
>> a mix of traffic. So have one NIC doing TCP_RR while another is doing
>> a stream test. You are stuffing 7 buffers onto a queue that were were
>> using to perform bulk freeing. How much of a penalty do you take if
>> you are now limited on how many you can bulk free because somebody
>> left a stray 7 packets sitting on the queue?
>
> Testing with TCP_RR, is not a very "clean" network test. One have to be
> very careful what is actually being tested, is it the server or client
> which is the bottleneck. And most of all this is test of the CPU/process
> scheduler.
>
> We can avoid the scheduler problem by enabling busy_poll/busy_read.
>
> I guess you want to see the "scheduler test" first. Default setting of
> disabled busy poll on both client and server:
>
> Disable busy poll on both client and server, Not patched:
Lets also define what "Not patched" means. I only want the bulk
allocation patch tested. The other patches you have for the mlx4 and
the WARN_ON are just noise. If possible it would be best to focus on
just the one patch that is high risk.
> $ netperf -H 198.18.40.2 -t TCP_RR -l 60 -T 6,6 -Cc
> MIGRATED TCP REQUEST/RESPONSE TEST from 0.0.0.0 (0.0.0.0) port 0 AF_INET to 198.18.40.2
> () port 0 AF_INET : histogram : demo : first burst 0 : cpu bind
> Local /Remote
> Socket Size Request Resp. Elapsed Trans. CPU CPU S.dem S.dem
> Send Recv Size Size Time Rate local remote local remote
> bytes bytes bytes bytes secs. per sec % S % S us/Tr us/Tr
>
> 16384 87380 1 1 60.00 78077.55 3.74 2.69 3.830 8.265
> 16384 87380
>
> Disable busy poll on both client and server, patched:
>
> $ netperf -H 198.18.40.2 -t TCP_RR -l 60 -T 6,6 -Cc
> MIGRATED TCP REQUEST/RESPONSE TEST from 0.0.0.0 (0.0.0.0) port 0 AF_INET to 198.18.40.2
> () port 0 AF_INET : histogram : demo : first burst 0 : cpu bind
> Local /Remote
> Socket Size Request Resp. Elapsed Trans. CPU CPU S.dem S.dem
> Send Recv Size Size Time Rate local remote local remote
> bytes bytes bytes bytes secs. per sec % S % S us/Tr us/Tr
>
> 16384 87380 1 1 60.00 78517.32 3.06 2.84 3.118 8.677
> 16384 87380
>
> I will not call this an improvement... the results are basically the same.
So I have a few suggestions.
1. Either switch to ixgbe and use ATR/Flow Director or look at setting
up your test so that the RSS key and indirection table are the same for
each test and use the "-- -P" option in netperf to force the use of the
same 5 tuple for each test.
2. Cut down on the noise. Specifically rebuild your kernel with as few
options enabled as possible. If you don't need it drop it out so that
we can identify exactly how much gain there is to be had from your
patches. Also you should increase your test to use multiple CPUs, or
cut down on the number of CPUs so that you aren't cutting down the the
CPU utilization so much. If you have to you might even look at doing
something like using SAR -P 6 in order to be able to monitor the CPU
utilization of just CPU 6 on your test system.
The goal is you want your tests to be repeatable if you had to move to
another system or another NIC so I would recommend trying to find a way
to make it so that much of the fluctuation is ruled out and that your
numbers are as reliable as possible.
3. You might even try a pktgen Rx and drop test to see what the
difference is in ns/packet for your allocation routine. Assuming you
can clear out the variability that would be a useful datapoint as you
could then also collect the perf data to show which functions have
reduced their total CPU time.
> Next step enabling busy poll on the server. The server is likely the
> bottleneck, given it's CPU is slower than the client. Context switches
> on the server is too high 156K/sec, after enabling busy poll reduced to
> 620/sec. Note the client is doing around 233k/sec context switches,
> (fairly impressive).
>
> Enabling busy poll on the server:
> sysctl -w net.core.busy_poll=50
> sysctl -w net.core.busy_read=50
>
>
> Enabled busy poll only on server, Not patched:
> $ netperf -H 198.18.40.2 -t TCP_RR -l 60 -T 6,6 -Cc
> MIGRATED TCP REQUEST/RESPONSE TEST from 0.0.0.0 (0.0.0.0) port 0 AF_INET to 198.18.40.2
> () port 0 AF_INET : histogram : demo : first burst 0 : cpu bind
> Local /Remote
> Socket Size Request Resp. Elapsed Trans. CPU CPU S.dem S.dem
> Send Recv Size Size Time Rate local remote local remote
> bytes bytes bytes bytes secs. per sec % S % S us/Tr us/Tr
>
> 16384 87380 1 1 60.00 112480.72 5.90 4.68 4.194 9.984
> 16384 87380
>
> Enabled busy poll only on server, patched:
>
> $ netperf -H 198.18.40.2 -t TCP_RR -l 60 -T 6,6 -Cc
> MIGRATED TCP REQUEST/RESPONSE TEST from 0.0.0.0 (0.0.0.0) port 0 AF_INET to 198.18.40.2
> () port 0 AF_INET : histogram : demo : first burst 0 : cpu bind
> Local /Remote
> Socket Size Request Resp. Elapsed Trans. CPU CPU S.dem S.dem
> Send Recv Size Size Time Rate local remote local remote
> bytes bytes bytes bytes secs. per sec % S % S us/Tr us/Tr
>
> 16384 87380 1 1 60.00 110152.34 5.84 4.60 4.242 10.014
> 16384 87380
>
> Numbers are too close, for any conclusions.
Agreed.
> Running a second run, on Not-patched kernel:
> Enabled busy poll only on server, Not patched:
> [jbrouer@canyon ~]$ netperf -H 198.18.40.2 -t TCP_RR -l 60 -T 6,6 -Cc
> MIGRATED TCP REQUEST/RESPONSE TEST from 0.0.0.0 (0.0.0.0) port 0 AF_INET to 198.18.40.2
> () port 0 AF_INET : histogram : demo : first burst 0 : cpu bind
> Local /Remote
> Socket Size Request Resp. Elapsed Trans. CPU CPU S.dem S.dem
> Send Recv Size Size Time Rate local remote local remote
> bytes bytes bytes bytes secs. per sec % S % S us/Tr us/Tr
>
> 16384 87380 1 1 60.00 101554.90 4.12 4.31 3.245 10.185
> 16384 87380
>
> Thus, variation between runs are bigger than any improvement/regression,
> thus no performance conclusions from this change can be drawn.
Like Eric mentioned this is likely the fact that you are bouncing
between Rx queues.
> Lets move beyond testing the CPU/process scheduler by enabling
> busy-polling on both client and server:
> (sysctl -w net.core.busy_poll=50 ;sysctl -w net.core.busy_read=50)
>
> Enable busy poll on both client and server, Not patched:
>
> $ netperf -H 198.18.40.2 -t TCP_RR -l 60 -T 6,6 -Cc
> MIGRATED TCP REQUEST/RESPONSE TEST from 0.0.0.0 (0.0.0.0) port 0 AF_INET to 198.18.40.2 () port 0 AF_INET : histogram : demo : first burst 0 : cpu bind
> Local /Remote
> Socket Size Request Resp. Elapsed Trans. CPU CPU S.dem S.dem
> Send Recv Size Size Time Rate local remote local remote
> bytes bytes bytes bytes secs. per sec % S % S us/Tr us/Tr
>
> 16384 87380 1 1 60.00 137987.86 13.18 4.77 7.643 8.298
> 16384 87380
>
>
> Enable busy poll on both client and server, patched:
>
> $ netperf -H 198.18.40.2 -t TCP_RR -l 60 -T 6,6 -Cc
> MIGRATED TCP REQUEST/RESPONSE TEST from 0.0.0.0 (0.0.0.0) port 0 AF_INET to 198.18.40.2 () port 0 AF_INET : histogram : demo : first burst 0 : cpu bind
> Local /Remote
> Socket Size Request Resp. Elapsed Trans. CPU CPU S.dem S.dem
> Send Recv Size Size Time Rate local remote local remote
> bytes bytes bytes bytes secs. per sec % S % S us/Tr us/Tr
>
> 16384 87380 1 1 60.00 147324.38 13.76 4.76 7.474 7.747
> 16384 87380
>
> I've a little bit surprised to see such a large improvement here 6.76%.
> 147324/137987*100 = 106.76
That is a difference of 500ns per packet. I am highly doubtful we are
seeing that much of an improvement as well. Odds are you were doing
something cross-node in your first run or something along those lines
since the difference is too large to be attributed to the bulk
allocation change.
> I'm remaining skeptic towards this measurement, as the improvement
> should not be this high. Even if recycling is happening.
>
> Perf record does show less calls to __slab_free(), indicating better
> interaction with SLUB, and perhaps recycling working. But this is
> only a perf-report change from 0.37% to 0.33%.
>
> More testing show not-patched kernel fluctuate between 125k-143k/sec,
> and patched kernel fluctuate between 131k-152k/sec. The ranges are too
> high, to say anything conclusive. It seems to be timing dependent, as
> starting and stoping the test with -D 1, show a rate variation within
> 2k/sec, but rate itself can vary withing the range stated.
I am pretty sure we are guaranteed to see a performance regression for
socket based workloads. You are hoping for recycling to occur, but in
almost all cases recycling almost always fails to show any gains and
just ends up introducing the possibility for regressions as something
always gets overlooked. On top of that you aren't guaranteed a frame
from Tx clean-up is going to be warm in the cache so you may end up
taking a few cache line misses as well.
I haven't seen enough change in these patches to justify having them
submitted to the kernel. A 1% improvement in one specific test case is
kind of a vague reason to do something that very likely introduces
regressions in many other cases.
- Alex
^ permalink raw reply
* Re: [PATCH net-next 01/14] qed: Add CONFIG_QED_SRIOV
From: Alexander Duyck @ 2016-05-10 18:02 UTC (permalink / raw)
To: Yuval Mintz; +Cc: David Miller, netdev, Ariel Elior
In-Reply-To: <CO2PR11MB00887DD775313A8DC3F39FA597710@CO2PR11MB0088.namprd11.prod.outlook.com>
On Tue, May 10, 2016 at 10:16 AM, Yuval Mintz <Yuval.Mintz@qlogic.com> wrote:
> > From: Yuval Mintz <Yuval.Mintz@qlogic.com>
>> Date: Mon, 9 May 2016 16:19:10 +0300
>>
>> > + /* bitmap indicating which fields hold valid values */
>> > + aligned_u64 valid_bitmap;
>>
>> There is absolutely no reason to use aligned_u64 here. That type is for handling
>> a specific issue in user facing APIs, which this is not.
>
> I'm not entirely convinced this is true; If we'll not enforce the alignment
> of this 64-bit field, it's possible there will be differences between 32-bit
> and 64-bit machines versions of this struct.
> You have to recall that this is going to be copied via DMA between PF and VF,
> so they must have the exact same representation of the structure.
>
> [If I'm wrong on the technical part here, please correct me; I vaguely
> seem to recall that this was already discussed on bnx2x's implementation
> of the hw-channel which also uses aligned u64 fields]
I think your change does have an impact, I just don't know if you
really realize what it will get you. Specifically what using the
aligned_u64 is doing is forcing qed_bulletin_content to be u64 aligned
and introducing two holes in qed_bulletin on 32 bit platforms where
dma_addr_t might be 32 bit, and adding a 4 bytes of padding after
size.
My advice would be to update phys to be a u64 instead of a dma_addr_t.
That way it won't change size depending on 32 or 64 bit architecture.
Then you could align qed_bulletin to 8 bytes so that the size of the
structure remains constant on both 32 and 64 bit systems.
- Alex
^ permalink raw reply
* Re: [PATCH net-next 01/14] qed: Add CONFIG_QED_SRIOV
From: David Miller @ 2016-05-10 18:06 UTC (permalink / raw)
To: Yuval.Mintz; +Cc: netdev, Ariel.Elior
In-Reply-To: <CO2PR11MB00887DD775313A8DC3F39FA597710@CO2PR11MB0088.namprd11.prod.outlook.com>
From: Yuval Mintz <Yuval.Mintz@qlogic.com>
Date: Tue, 10 May 2016 17:16:16 +0000
> I'm not entirely convinced this is true; If we'll not enforce the alignment
> of this 64-bit field, it's possible there will be differences between 32-bit
> and 64-bit machines versions of this struct.
> You have to recall that this is going to be copied via DMA between PF and VF,
> so they must have the exact same representation of the structure.
Then use properly sized types to fill in all the space in the
structure, that's how you guarantee layout, not aligned_u64. Also, do
not use the packed attribute.
struct foo {
u32 x;
u32 y;
u64 z;
};
'z' will always be 64-bit aligned.
^ permalink raw reply
* Re: [PATCH net-next 01/14] qed: Add CONFIG_QED_SRIOV
From: David Miller @ 2016-05-10 18:09 UTC (permalink / raw)
To: alexander.duyck; +Cc: Yuval.Mintz, netdev, Ariel.Elior
In-Reply-To: <CAKgT0UdvLpAxcCuNHZy6BJQyDiDOkpoJprjBXLM-43N5pGXOOg@mail.gmail.com>
From: Alexander Duyck <alexander.duyck@gmail.com>
Date: Tue, 10 May 2016 11:02:09 -0700
> On Tue, May 10, 2016 at 10:16 AM, Yuval Mintz <Yuval.Mintz@qlogic.com> wrote:
>> > From: Yuval Mintz <Yuval.Mintz@qlogic.com>
>>> Date: Mon, 9 May 2016 16:19:10 +0300
>>>
>>> > + /* bitmap indicating which fields hold valid values */
>>> > + aligned_u64 valid_bitmap;
>>>
>>> There is absolutely no reason to use aligned_u64 here. That type is for handling
>>> a specific issue in user facing APIs, which this is not.
>>
>> I'm not entirely convinced this is true; If we'll not enforce the alignment
>> of this 64-bit field, it's possible there will be differences between 32-bit
>> and 64-bit machines versions of this struct.
>> You have to recall that this is going to be copied via DMA between PF and VF,
>> so they must have the exact same representation of the structure.
>>
>> [If I'm wrong on the technical part here, please correct me; I vaguely
>> seem to recall that this was already discussed on bnx2x's implementation
>> of the hw-channel which also uses aligned u64 fields]
>
> I think your change does have an impact, I just don't know if you
> really realize what it will get you. Specifically what using the
> aligned_u64 is doing is forcing qed_bulletin_content to be u64 aligned
> and introducing two holes in qed_bulletin on 32 bit platforms where
> dma_addr_t might be 32 bit, and adding a 4 bytes of padding after
> size.
dma_addr_t is 32-bits on some 64-bit architectures too.
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox