* [PATCH net-next v2] ila: ipv6/ila: fix nlsize calculation for lwtunnel
From: Nicolas Dichtel @ 2016-05-10 9:56 UTC (permalink / raw)
To: tom; +Cc: netdev, davem, Nicolas Dichtel
In-Reply-To: <20160501.193926.994161705403597197.davem@davemloft.net>
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
^ permalink raw reply related
* Re: [Drbd-dev] [PATCH net-next v3] block/drbd: align properly u64 in nl messages
From: Nicolas Dichtel @ 2016-05-10 10:06 UTC (permalink / raw)
To: davem, netdev, philipp.reisner, drbd-dev, linux-kernel
In-Reply-To: <20160510094023.GC16459@soda.linbit>
Le 10/05/2016 11:40, Lars Ellenberg a écrit :
> On Tue, May 10, 2016 at 11:09:53AM +0200, Nicolas Dichtel wrote:
>> Le 09/05/2016 15:15, Lars Ellenberg a écrit :
>>> On Mon, May 09, 2016 at 11:40:20AM +0200, Nicolas Dichtel wrote:
>> [snip]
>>>> Maybe prefixing genl_magic_func.h and genl_magic_struct.h by 'drbd_'
>>>> could be interesting so that new module won't use it. What is your
>>>> opinion?
>>>
>>> This was supposed to not be DRBD specific. But it might even still
>>> need some massaging before it was truly generic. And obviously,
>>> it does not meet the taste of genetlink folks, to say the least :(
>> Yes, this file is not generic and netlink APIs are never defined like this.
>> These tons of macro complexifies the code too much. It's overengineering for
>> what purpose?
>
> 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.
Ok.
>
>> Small examples:
>> - the drbd netlink API is not exported via uapi (I wonder how apps using this
>> API get it)
>
> There used to be a time where there was no "uapi".
> (I wonder how apps ever worked back then).
At that time, include/linux/ was exported ;-)
>
>> - v2 of the patch is nacked because adding a new attribute may break existing
>
> No.
>
> But because the "new" attributes you chose have not been new,
> but already used (though not yet merged back into mainline yet).
> (Which you did not realize, and had no obvious way of knowing.
> Could have been fixed.).
Ok.
>
> And because your patch introduced useless new members to the structs.
> (Could also have been fixed).
>
> And because I did not see any use defining that many new "padding attributes"
> for no reason, where the obvious (to me) choice was to use 0, and you
> did not even try to explain why that would have been a bad choice.
Because some nl APIs were wrongly use 0 as a valid attribute we make the choice
of always adding a new attribute for padding to be sure to not break existing API.
And yes, in drdb it does not seem to be the case.
> Is this going somewhere?
I'm just trying to understand things.
Regards,
Nicolas
^ permalink raw reply
* [PATCH] net: phylib: fix interrupts re-enablement in phy_start
From: shh.xie @ 2016-05-10 9:42 UTC (permalink / raw)
To: netdev, davem, f.fainelli; +Cc: Shaohui Xie
From: Shaohui Xie <Shaohui.Xie@nxp.com>
If phy was suspended and is starting, current driver always enable
phy's interrupts, if phy works in polling, phy can raise unexpected
interrupt which will not be handled, the interrupt will block system
enter suspend again. So interrupts should only be re-enabled if phy
works in interrupt.
Signed-off-by: Shaohui Xie <Shaohui.Xie@nxp.com>
---
drivers/net/phy/phy.c | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index 6f221c8..baa5ecb 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -871,9 +871,11 @@ void phy_start(struct phy_device *phydev)
break;
case PHY_HALTED:
/* make sure interrupts are re-enabled for the PHY */
- err = phy_enable_interrupts(phydev);
- if (err < 0)
- break;
+ if (phydev->irq != PHY_POLL) {
+ err = phy_enable_interrupts(phydev);
+ if (err < 0)
+ break;
+ }
phydev->state = PHY_RESUMING;
do_resume = true;
--
2.1.0.27.g96db324
^ permalink raw reply related
* RE: [PATCH net-next 5/7] Driver: Vmxnet3: Add support for get_coalesce, set_coalesce ethtool operations
From: David Laight @ 2016-05-10 11:24 UTC (permalink / raw)
To: 'Ben Hutchings', Shrikrishna Khare
Cc: netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
pv-drivers@vmware.com, Keyong Sun, Manoj Tammali
In-Reply-To: <1462753040.9308.11.camel@decadent.org.uk>
From: Ben Hutchings
> Sent: 09 May 2016 01:17
> On Sun, 2016-05-08 at 13:55 -0700, Shrikrishna Khare wrote:
> >
> > On Sat, 7 May 2016, Ben Hutchings wrote:
> >
> > > On Fri, 2016-05-06 at 16:12 -0700, Shrikrishna Khare wrote:
> > > [...]
> > > > +static int
> > > > +vmxnet3_set_coalesce(struct net_device *netdev, struct ethtool_coalesce *ec)
> > > > +{
> > > [...]
> > > > + switch (ec->rx_coalesce_usecs) {
> > > > + case VMXNET3_COALESCE_DEFAULT:
> > > > + case VMXNET3_COALESCE_DISABLED:
> > > > + case VMXNET3_COALESCE_ADAPT:
> > > > + if (ec->tx_max_coalesced_frames ||
> > > > + ec->tx_max_coalesced_frames_irq ||
> > > > + ec->rx_max_coalesced_frames_irq) {
> > > > + return -EINVAL;
> > > > + }
> > > > + memset(adapter->coal_conf, 0, sizeof(*adapter->coal_conf));
> > > > + adapter->coal_conf->coalMode = ec->rx_coalesce_usecs;
> > > > + break;
> > > > + case VMXNET3_COALESCE_STATIC:
> > > [...]
> > >
> > > I don't want to see drivers introducing magic values for fields that
> > > are denominated in microseconds (especially not for 0, which is the
> > > correct way to specify 'no coalescing'). If the current
> > > ethtool_coalesce structure is inadequate, propose an extension.
> >
> > For vmxnet3, we need an ethtool mechanism to indicate coalescing mode to
> > the device.
> >
> > Would a patch that maps 0 to 'no coalescing' be acceptable? That is:
> >
> > rx-usecs = 0 -> coalescing disabled.
> > rx-usecs = 1 -> default (chosen by the device).
> > rx-usecs = 2 -> adaptive coalescing.
> > rx-usecs = 3 -> static coalescing.
Would it be better to use stupidly large values for the non-zero special values?
That would less problematic if someone expects 2 to mean '2 usecs'.
David
^ permalink raw reply
* Re: [PATCH v9 net-next 4/7] openvswitch: add layer 3 flow/port support
From: Jiri Benc @ 2016-05-10 12:01 UTC (permalink / raw)
To: Simon Horman; +Cc: ovs dev, Linux Kernel Network Developers
In-Reply-To: <20160509080420.GA4470-IxS8c3vjKQDk1uMJSBkQmQ@public.gmane.org>
On Mon, 9 May 2016 17:04:22 +0900, Simon Horman wrote:
> It seems to be caused by the following:
>
> 1. __ipgre_rcv() calls skb_pop_mac_header() which
> sets skb->mac_header to the skb->network_header.
>
> 2. __ipgre_rcv() then calls ip_tunnel_rcv() which calls
> skb_reset_network_header(). This updates skb->network_header to
> just after the end of the GRE header.
>
> This is 28 bytes after the old skb->network_header
> as there is a 20 byte IPv4 header followed by an
> 8 byte GRE header.
>
> 3. Later, dev_gro_receive() calls skb_reset_mac_len().
> This calculates skb->mac_len based on skb->network_header and
> skb->mac_header. I.e. 28 bytes.
Right. Thanks for tracking this down!
> I think this may be possible to address by calling
> skb_reset_network_header() instead of skb_pop_mac_header()
> in __ipgre_rcv().
We can't do that. The interface type is ARPHRD_IPGRE and not
ARPHRD_NONE, so the current behavior makes pretty good sense. See
e.g. commit 0e3da5bb8da45.
We have two options here:
1. As for metadata tunnels all the info is in metadata_dst and we
don't need the IP/GRE header for anything, we can make the ipgre
interface ARPHRD_NONE in metadata based mode.
2. We can fix this up in ovs after receiving the packet from
ARPHRD_IPGRE interface.
I think the first option is the correct one. We already don't assign
dev->header_ops in metadata mode. I'll prepare a patch.
> Its possible that I've overlooked something but as things stand I think
> things look like this:
>
> * ovs_flow_key_extract() keys off dev->type and skb->protocol.
> * ovs_flow_key_extract() calls key_extract() which amongst other things
> sets up the skb->mac_header and skb->mac_len of the skb.
> * ovs_flow_key_extract() sets skb->protocol to that of the inner packet
> in the case of TEB
> * Actions update the above mentioned skb fields as appropriate.
Okay, that actually eases things somewhat.
> So it seems to me that it should be safe to rely on skb->protocol
> in the receive path. Or more specifically, in netdev_port_receive().
>
> If mac_len is also able to be used then I think fine. But it seems to me
> that it needs to be set up by OvS at least for the ARPHRD_NONE case. This
> could be done early on, say in netdev_port_receive(). But it seems that
> would involve duplicating some of what is already occurring in
> key_extract().
I'd actually prefer doing this earlier, netdev_port_receive looks like
the right place. Just set mac_len there (or whatever) and let
key_extract do the rest of the work, not depending on dev->type in
there.
My point about recirculation was not actually valid, as I missed you're
doing this in ovs_flow_key_extract and not in key_extract. Still,
I think the special handling of particular interface types belongs to
the tx processing on those interfaces, not to the common code.
Thanks!
Jiri
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
^ permalink raw reply
* Re: [PATCH net-next 5/7] Driver: Vmxnet3: Add support for get_coalesce, set_coalesce ethtool operations
From: Ben Hutchings @ 2016-05-10 12:02 UTC (permalink / raw)
To: David Laight, Shrikrishna Khare
Cc: netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
pv-drivers@vmware.com, Keyong Sun, Manoj Tammali
In-Reply-To: <063D6719AE5E284EB5DD2968C1650D6D5F4B8B61@AcuExch.aculab.com>
[-- Attachment #1: Type: text/plain, Size: 2564 bytes --]
On Tue, 2016-05-10 at 11:24 +0000, David Laight wrote:
> From: Ben Hutchings
> >
> > Sent: 09 May 2016 01:17
> > On Sun, 2016-05-08 at 13:55 -0700, Shrikrishna Khare wrote:
> > >
> > >
> > > On Sat, 7 May 2016, Ben Hutchings wrote:
> > >
> > > >
> > > > On Fri, 2016-05-06 at 16:12 -0700, Shrikrishna Khare wrote:
> > > > [...]
> > > > >
> > > > > +static int
> > > > > +vmxnet3_set_coalesce(struct net_device *netdev, struct ethtool_coalesce *ec)
> > > > > +{
> > > > [...]
> > > > >
> > > > > + switch (ec->rx_coalesce_usecs) {
> > > > > + case VMXNET3_COALESCE_DEFAULT:
> > > > > + case VMXNET3_COALESCE_DISABLED:
> > > > > + case VMXNET3_COALESCE_ADAPT:
> > > > > + if (ec->tx_max_coalesced_frames ||
> > > > > + ec->tx_max_coalesced_frames_irq ||
> > > > > + ec->rx_max_coalesced_frames_irq) {
> > > > > + return -EINVAL;
> > > > > + }
> > > > > + memset(adapter->coal_conf, 0, sizeof(*adapter->coal_conf));
> > > > > + adapter->coal_conf->coalMode = ec->rx_coalesce_usecs;
> > > > > + break;
> > > > > + case VMXNET3_COALESCE_STATIC:
> > > > [...]
> > > >
> > > > I don't want to see drivers introducing magic values for fields that
> > > > are denominated in microseconds (especially not for 0, which is the
> > > > correct way to specify 'no coalescing'). If the current
> > > > ethtool_coalesce structure is inadequate, propose an extension.
> > > For vmxnet3, we need an ethtool mechanism to indicate coalescing mode to
> > > the device.
> > >
> > > Would a patch that maps 0 to 'no coalescing' be acceptable? That is:
> > >
> > > rx-usecs = 0 -> coalescing disabled.
> > > rx-usecs = 1 -> default (chosen by the device).
> > > rx-usecs = 2 -> adaptive coalescing.
> > > rx-usecs = 3 -> static coalescing.
> Would it be better to use stupidly large values for the non-zero special values?
> That would less problematic if someone expects 2 to mean '2 usecs'.
That seems less problematic, but I think we'd have to check that
current drivers (or at least the most widely used ones) reject those
values.
We should still need to standardise names and definitions for those
special values, so that the ethtool utility can provide keywords and
documentation for them.
Ben.
--
Ben Hutchings
If you seem to know what you are doing, you'll be given more to do.
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [PATCH v9 net-next 4/7] openvswitch: add layer 3 flow/port support
From: Jiri Benc @ 2016-05-10 12:06 UTC (permalink / raw)
To: Simon Horman; +Cc: netdev, dev, Lorand Jakab, Thomas Morin
In-Reply-To: <20160509081818.GB4470@vergenet.net>
On Mon, 9 May 2016 17:18:20 +0900, Simon Horman wrote:
> On Fri, May 06, 2016 at 11:35:04AM +0200, Jiri Benc wrote:
> > In addition, we should check whether mac_len > 0 and in such case,
> > change skb->protocol to ETH_P_TEB first (and store that value in the
> > pushed eth header).
> >
> > Similarly on pop_eth, we need to check skb->protocol and if it is
> > ETH_P_TEB, call eth_type_trans on the modified frame to set the new
> > skb->protocol correctly. It's probably not that simple, as we'd need a
> > version of eth_type_trans that doesn't need a net device.
>
> I'm not sure I understand the interaction with ETH_P_TEB here.
>
> In my mind skb->protocol == ETH_P_TEB may be used early on in OvS's receive
> processing to find the inner protocol from the packet and at that point
> skb->protocol is set to that value. And that for further packet processing
> the fact the packet was received as TEB is transparent.
Yes but think about the case when you have two Ethernet headers pushed.
We can either disallow it, or do what I described.
Specifically, let's assume we have an IP packet with an Ethernet
header present. skb->protocol is ETH_P_IP. Now, when there's skb_push,
the correct operation would be setting skb->protocol to ETH_P_TEB,
pushing a new Ethernet header and filing ETH_P_TEB to the ethertype
field in the new header. The packet is not ETH_P_IP anymore, as the L2
header is followed by another Ethernet header now.
Jiri
^ permalink raw reply
* Re: [PATCH v9 net-next 4/7] openvswitch: add layer 3 flow/port support
From: Jiri Benc @ 2016-05-10 12:07 UTC (permalink / raw)
To: Yang, Yi Y
Cc: dev-yBygre7rU0TnMu66kgdUjQ@public.gmane.org, Simon Horman,
netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
In-Reply-To: <79BBBFE6CB6C9B488C1A45ACD284F51913CB6446-0J0gbvR4kTggGBtAFL8yw7fspsVTdybXVpNB7YpNyf8@public.gmane.org>
Please do not top post.
On Tue, 10 May 2016 00:16:36 +0000, Yang, Yi Y wrote:
> I think it is still necessary to keep eth_type in push_eth action, for
> the classifier case, it will push_nsh then push_eth for the original
> frame, this will need to set eth_type to 0x894f by push_eth, otherwise
> push_eth won't know this eth_type.
Nope, push_nsh will set skb->protocol to ETH_P_NSH. Later push_eth will
use that value correctly.
Jiri
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
^ permalink raw reply
* Re: [net-next PATCH V1 1/3] net: bulk alloc and reuse of SKBs in NAPI context
From: Jesper Dangaard Brouer @ 2016-05-10 12:30 UTC (permalink / raw)
To: Alexander Duyck
Cc: Netdev, David S. Miller, Saeed Mahameed, Or Gerlitz,
Eugenia Emantayev, brouer
In-Reply-To: <CAKgT0UfKzKWnNzGpB-915by2M1nzDAdNz-hDwwcwGoowmZefrg@mail.gmail.com>
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:
$ 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.
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.
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.
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
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.
--
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] fjes: Fix unnecessary spinlock_irqsave
From: Sergei Shtylyov @ 2016-05-10 13:13 UTC (permalink / raw)
To: Taku Izumi, netdev, davem; +Cc: isimatu.yasuaki
In-Reply-To: <1462843729-32414-1-git-send-email-izumi.taku@jp.fujitsu.com>
Hello.
On 5/10/2016 4:28 AM, Taku Izumi wrote:
> commit-bd5a256 introduces a deadlock bug in fjes_change_mtu().
scripts/checkpatch.pl now enforces a certain commit citing style, yours
doesn't match it.
> This spin_lock_irqsave() is obviously unnecessary.
>
> This patch eliminates unnecessary spin_lock_irqsave() in
> fjes_change_mtu()
>
> Signed-off-by: Taku Izumi <izumi.taku@jp.fujitsu.com>
[...]
The "Fixes:" tag would also be in order...
MBR, Sergei
^ permalink raw reply
* Re: [PATCH net-next 1/4] xen-netback: add control ring boilerplate
From: Wei Liu @ 2016-05-10 13:29 UTC (permalink / raw)
To: Paul Durrant; +Cc: xen-devel, netdev, Wei Liu
In-Reply-To: <1462447170-1815-2-git-send-email-paul.durrant@citrix.com>
On Thu, May 05, 2016 at 12:19:27PM +0100, Paul Durrant wrote:
[...]
>
> +static int connect_ctrl_ring(struct backend_info *be)
> +{
Please use goto style error handling in this function.
Other than this the code looks good.
Wei.
^ permalink raw reply
* Re: [PATCH net-next 4/4] xen-netback: use hash value from the frontend
From: Wei Liu @ 2016-05-10 13:29 UTC (permalink / raw)
To: Paul Durrant; +Cc: xen-devel, netdev, Wei Liu
In-Reply-To: <1462447170-1815-5-git-send-email-paul.durrant@citrix.com>
On Thu, May 05, 2016 at 12:19:30PM +0100, Paul Durrant wrote:
> My recent patch to include/xen/interface/io/netif.h defines a new extra
> info type that can be used to pass hash values between backend and guest
> frontend.
>
> This patch adds code to xen-netback to use the value in a hash extra
> info fragment passed from the guest frontend in a transmit-side
> (i.e. netback receive side) packet to set the skb hash accordingly.
>
> Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
> Cc: Wei Liu <wei.liu2@citrix.com>
Acked-by: Wei Liu <wei.liu2@citrix.com>
^ permalink raw reply
* Re: [PATCH net-next 2/4] xen-netback: add control protocol implementation
From: Wei Liu @ 2016-05-10 13:29 UTC (permalink / raw)
To: Paul Durrant; +Cc: xen-devel, netdev, Wei Liu
In-Reply-To: <1462447170-1815-3-git-send-email-paul.durrant@citrix.com>
On Thu, May 05, 2016 at 12:19:28PM +0100, Paul Durrant wrote:
> My recent patch to include/xen/interface/io/netif.h defines a new shared
> ring (in addition to the rx and tx rings) for passing control messages
> from a VM frontend driver to a backend driver.
>
> A previous patch added the necessary boilerplate for mapping the control
> ring from the frontend, should it be created. This patch adds
> implementations for each of the defined protocol messages.
>
> Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
> Cc: Wei Liu <wei.liu2@citrix.com>
> ---
> drivers/net/xen-netback/Makefile | 2 +-
> drivers/net/xen-netback/common.h | 43 +++++
> drivers/net/xen-netback/hash.c | 361 ++++++++++++++++++++++++++++++++++++
> drivers/net/xen-netback/interface.c | 28 +++
> drivers/net/xen-netback/netback.c | 49 ++++-
> 5 files changed, 480 insertions(+), 3 deletions(-)
> create mode 100644 drivers/net/xen-netback/hash.c
>
Other than the issue mentioned by David, the code looks OK to me.
Wei.
^ permalink raw reply
* Re: [PATCH net-next 3/4] xen-netback: pass hash value to the frontend
From: Wei Liu @ 2016-05-10 13:29 UTC (permalink / raw)
To: Paul Durrant; +Cc: xen-devel, netdev, Wei Liu
In-Reply-To: <1462447170-1815-4-git-send-email-paul.durrant@citrix.com>
On Thu, May 05, 2016 at 12:19:29PM +0100, Paul Durrant wrote:
> My recent patch to include/xen/interface/io/netif.h defines a new extra
> info type that can be used to pass hash values between backend and guest
> frontend.
>
> This patch adds code to xen-netback to pass hash values calculated for
> guest receive-side packets (i.e. netback transmit side) to the frontend.
>
> Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
> Cc: Wei Liu <wei.liu2@citrix.com>
Acked-by: Wei Liu <wei.liu2@citrix.com>
^ permalink raw reply
* [PATCH nf] netfilter: fix oops in nfqueue during netns error unwinding
From: Florian Westphal @ 2016-05-10 13:35 UTC (permalink / raw)
To: netfilter-devel; +Cc: netdev, Florian Westphal, Eric W. Biederman
Under full load (unshare() in loop -> OOM conditions) we can
get kernel panic:
BUG: unable to handle kernel NULL pointer dereference at 0000000000000008
IP: [<ffffffff81476c85>] nfqnl_nf_hook_drop+0x35/0x70
[..]
task: ffff88012dfa3840 ti: ffff88012dffc000 task.ti: ffff88012dffc000
RIP: 0010:[<ffffffff81476c85>] [<ffffffff81476c85>] nfqnl_nf_hook_drop+0x35/0x70
RSP: 0000:ffff88012dfffd80 EFLAGS: 00010206
RAX: 0000000000000008 RBX: ffffffff81add0c0 RCX: ffff88013fd80000
[..]
Call Trace:
[<ffffffff81474d98>] nf_queue_nf_hook_drop+0x18/0x20
[<ffffffff814738eb>] nf_unregister_net_hook+0xdb/0x150
[<ffffffff8147398f>] netfilter_net_exit+0x2f/0x60
[<ffffffff8141b088>] ops_exit_list.isra.4+0x38/0x60
[<ffffffff8141b652>] setup_net+0xc2/0x120
[<ffffffff8141bd09>] copy_net_ns+0x79/0x120
[<ffffffff8106965b>] create_new_namespaces+0x11b/0x1e0
[<ffffffff810698a7>] unshare_nsproxy_namespaces+0x57/0xa0
[<ffffffff8104baa2>] SyS_unshare+0x1b2/0x340
[<ffffffff81608276>] entry_SYSCALL_64_fastpath+0x1e/0xa8
Code: 65 00 48 89 e5 41 56 41 55 41 54 53 83 e8 01 48 8b 97 70 12 00 00 48 98 49 89 f4 4c 8b 74 c2 18 4d 8d 6e 08 49 81 c6 88 00 00 00 <49> 8b 5d 00 48 85 db 74 1a 48 89 df 4c 89 e2 48 c7 c6 90 68 47
Problem is that we call into the nfqueue backend to zap
packets that might be queued to userspace.
However, this assumes that the backend was initialized and
net_generic(net, nfnl_queue_net_id) returns valid memory.
Unfortunately this isn't the case; its possible that we e.g. failed to
create the nfqueue proc directory. In this case the returned memory
was already free'd and we oops when we try to grab the instance lock.
Add a marker to the netfilter pernetns data that tells nf_queue if
the backend was initialized in this namespace.
Cc: "Eric W. Biederman" <ebiederm@xmission.com>
Fixes: 8405a8fff3f ("netfilter: nf_qeueue: Drop queue entries on nf_unregister_hook")
Signed-off-by: Florian Westphal <fw@strlen.de>
---
I don't like this fix, but I am not sure what other (reliable!) alternatives
exist.
Another solution might be to alter
net_namespace.c:ops_init so that it sets
net_assign_generic(net, *ops->id, NULL);
... when the ops->init hook returns an error.
We'd need to also add bounds check to net_generic(), or do this
check in nfnetlink_queue to make sure ptr[id] can be accessed.
Other ideas?
include/net/netns/netfilter.h | 5 +++++
net/netfilter/nf_queue.c | 2 +-
net/netfilter/nfnetlink_queue.c | 1 +
3 files changed, 7 insertions(+), 1 deletion(-)
diff --git a/include/net/netns/netfilter.h b/include/net/netns/netfilter.h
index 38aa498..de75700 100644
--- a/include/net/netns/netfilter.h
+++ b/include/net/netns/netfilter.h
@@ -15,5 +15,10 @@ struct netns_nf {
struct ctl_table_header *nf_log_dir_header;
#endif
struct list_head hooks[NFPROTO_NUMPROTO][NF_MAX_HOOKS];
+
+ /* needed so nf core knows that we might need to drop
+ * queued packets on hook unregister
+ */
+ bool nfqueue_inited;
};
#endif
diff --git a/net/netfilter/nf_queue.c b/net/netfilter/nf_queue.c
index 5baa8e2..b6d585d 100644
--- a/net/netfilter/nf_queue.c
+++ b/net/netfilter/nf_queue.c
@@ -104,7 +104,7 @@ void nf_queue_nf_hook_drop(struct net *net, struct nf_hook_ops *ops)
rcu_read_lock();
qh = rcu_dereference(queue_handler);
- if (qh)
+ if (qh && net->nf.nfqueue_inited)
qh->nf_hook_drop(net, ops);
rcu_read_unlock();
}
diff --git a/net/netfilter/nfnetlink_queue.c b/net/netfilter/nfnetlink_queue.c
index cb5b630..536bac7 100644
--- a/net/netfilter/nfnetlink_queue.c
+++ b/net/netfilter/nfnetlink_queue.c
@@ -1377,6 +1377,7 @@ static int __net_init nfnl_queue_net_init(struct net *net)
net->nf.proc_netfilter, &nfqnl_file_ops))
return -ENOMEM;
#endif
+ net->nf.nfqueue_inited = true;
return 0;
}
--
2.7.3
^ permalink raw reply related
* RE: [PATCH net-next 2/4] xen-netback: add control protocol implementation
From: Paul Durrant @ 2016-05-10 13:46 UTC (permalink / raw)
To: Wei Liu; +Cc: xen-devel@lists.xenproject.org, netdev@vger.kernel.org, Wei Liu
In-Reply-To: <20160510132918.GD12241@citrix.com>
> -----Original Message-----
> From: Wei Liu [mailto:wei.liu2@citrix.com]
> Sent: 10 May 2016 14:29
> To: Paul Durrant
> Cc: xen-devel@lists.xenproject.org; netdev@vger.kernel.org; Wei Liu
> Subject: Re: [PATCH net-next 2/4] xen-netback: add control protocol
> implementation
>
> On Thu, May 05, 2016 at 12:19:28PM +0100, Paul Durrant wrote:
> > My recent patch to include/xen/interface/io/netif.h defines a new shared
> > ring (in addition to the rx and tx rings) for passing control messages
> > from a VM frontend driver to a backend driver.
> >
> > A previous patch added the necessary boilerplate for mapping the control
> > ring from the frontend, should it be created. This patch adds
> > implementations for each of the defined protocol messages.
> >
> > Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
> > Cc: Wei Liu <wei.liu2@citrix.com>
> > ---
> > drivers/net/xen-netback/Makefile | 2 +-
> > drivers/net/xen-netback/common.h | 43 +++++
> > drivers/net/xen-netback/hash.c | 361
> ++++++++++++++++++++++++++++++++++++
> > drivers/net/xen-netback/interface.c | 28 +++
> > drivers/net/xen-netback/netback.c | 49 ++++-
> > 5 files changed, 480 insertions(+), 3 deletions(-)
> > create mode 100644 drivers/net/xen-netback/hash.c
> >
>
> Other than the issue mentioned by David, the code looks OK to me.
>
Cool, thanks. I should have the RCU-based hash code done in the next day or so.
Paul
> Wei.
^ permalink raw reply
* RE: [PATCH net-next 1/4] xen-netback: add control ring boilerplate
From: Paul Durrant @ 2016-05-10 13:46 UTC (permalink / raw)
To: Wei Liu; +Cc: xen-devel@lists.xenproject.org, netdev@vger.kernel.org, Wei Liu
In-Reply-To: <20160510132915.GC12241@citrix.com>
> -----Original Message-----
> From: Wei Liu [mailto:wei.liu2@citrix.com]
> Sent: 10 May 2016 14:29
> To: Paul Durrant
> Cc: xen-devel@lists.xenproject.org; netdev@vger.kernel.org; Wei Liu
> Subject: Re: [PATCH net-next 1/4] xen-netback: add control ring boilerplate
>
> On Thu, May 05, 2016 at 12:19:27PM +0100, Paul Durrant wrote:
> [...]
> >
> > +static int connect_ctrl_ring(struct backend_info *be)
> > +{
>
> Please use goto style error handling in this function.
>
Will do.
> Other than this the code looks good.
>
Thanks,
Paul
> Wei.
^ 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 13:48 UTC (permalink / raw)
To: Jesper Dangaard Brouer
Cc: Alexander Duyck, Netdev, David S. Miller, Saeed Mahameed,
Or Gerlitz, Eugenia Emantayev
In-Reply-To: <20160510143017.212c3846@redhat.com>
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 (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.
^ permalink raw reply
* [PATCH net-next] ipv6: fix 4in6 tunnel receive path
From: Nicolas Dichtel @ 2016-05-10 14:08 UTC (permalink / raw)
To: davem; +Cc: netdev, Nicolas Dichtel, Tom Herbert
Protocol for 4in6 tunnel is IPPROTO_IPIP. This was wrongly changed by
the last cleanup.
CC: Tom Herbert <tom@herbertland.com>
Fixes: 0d3c703a9d17 ("ipv6: Cleanup IPv6 tunnel receive path")
Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
---
net/ipv6/ip6_tunnel.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/ipv6/ip6_tunnel.c b/net/ipv6/ip6_tunnel.c
index 50af7061ecdb..e79330f214bd 100644
--- a/net/ipv6/ip6_tunnel.c
+++ b/net/ipv6/ip6_tunnel.c
@@ -897,7 +897,7 @@ drop:
static int ip4ip6_rcv(struct sk_buff *skb)
{
- return ipxip6_rcv(skb, IPPROTO_IP, &tpi_v4,
+ return ipxip6_rcv(skb, IPPROTO_IPIP, &tpi_v4,
ip4ip6_dscp_ecn_decapsulate);
}
--
2.8.1
^ permalink raw reply related
* [RFC PATCH 0/2] net: threadable napi poll loop
From: Paolo Abeni @ 2016-05-10 14:11 UTC (permalink / raw)
To: netdev
Cc: 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
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.
This patch series allows the administrator to let the napi poll loop run inside
its own kernel thread, a thread for each napi instance, while retaining the
default, softirq-based behavior. The RPS mechanism is currently not affected.
When the napi poll loop is run inside a proper kernel thread, the process
scheduler can fairly balance the rx job between the user space application and
the kernel and give the administrator the ability to manage the network workload
with scheduler tools and configuration.
With the default scheduling policy, the starvation issue observed on single vCPU
guest under UDP flood is solved and the throughput measured under heavy
overload is quite stable around the peak performance.
In the remote host to VM scenario, running even the hypervisor napi poll loop
in threaded mode gives additional benefit, since the process scheduler can
more easily avoid cpu conflict between the VM process and the kernel thread
processing the rx packets.
The raw numbers, obtained with the super_neterf UDP_STREAM test, in a remote
host to VM scenario, using a tun device with a noqueue qdisc in the hypervisor
and using 'sdfn' for the rx flow hash on the ingress device, are as follow:
vanilla guest threaded both hypevisor and
guest threaded
size/flow kpps kpps/delta kpps/delta
1/1 746 901/+20% 1024/+37%
1/25 185 585/+215% 789/+325%
1/50 330 642/+94% 843/+155%
1/100 180 662/+267% 872/+383%
1/200 177 672/+279% 812/+358%
64/1 707 1042/+47% 1062/+50%
64/25 320 586/+83% 746/+132%
64/50 195 648/+232% 761/+290%
64/100 221 666/+200% 787/+255%
64/200 186 688/+268% 793/+325%
256/1 475 777/+63% 809/+70%
256/25 303 589/+83% 860/+183%
256/50 308 584/+89% 825/+168%
256/100 268 698/+159% 785/+191%
256/200 186 656/+398% 795/+503%
1438/1 619 664/+7% 640/+3%
1438/25 519 766/+47% 829/+59%
1438/50 451 712/+57% 820/+81%
1438/100 294 759/+158% 797/+170%
1438/200 262 728/+177% 769/+193%
4096/1 176 207/+17% 200/+13%
4096/25 225 275/+22% 286/+27%
4096/50 212 272/+28% 283/+33%
4096/100 168 264/+57% 283/+68%
4096/200 134 240/+78% 273/+102%
64000/1 16 18/+13% 18/+13%
64000/25 18 18/0 18/0
64000/50 18 18/0 18/0
64000/100 18 18/0 18/0
64000/200 15 15/0 15/0
This patchset is a first RFC but in the long run we would like to move
more and more NAPI instances into kthreads. The kthread approach should
give a lot of new advantages over the softirq based approach:
* moving into a more dpdk-alike busy poll packet processing direction:
we can even use busy polling without the need of a connected UDP or TCP
socket and can leverage busy polling for forwarding setups. This could
very well increase latency and packet throughput without hurting other
processes if the networking stack gets more and more preemptive in the
future.
* possibility to acquire mutexes in the networking processing path: e.g.
we would need that to configure hw_breakpoints if we want to add
watchpoints in the memory based on some rules in the kernel
* more and better tooling to adjust the weight of the networking
kthreads, preferring certain networking cards or setting cpus affinity
on packet processing threads. Maybe also using deadline scheduling or
other scheduler features might be worthwhile.
* scheduler statistics can be used to observe network packet processing
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.
[1] when the flows are processed by the hypervisor on different rx queues, i.e.
the flows use different source/destination IPs or the hypervisor uses the L4
header to compute the rx hash.
Paolo Abeni (2):
net: implement threaded-able napi poll loop support
net: add sysfs attribute to control napi threaded mode
include/linux/netdevice.h | 4 ++
net/core/dev.c | 113 ++++++++++++++++++++++++++++++++++++++++++++++
net/core/net-sysfs.c | 102 +++++++++++++++++++++++++++++++++++++++++
3 files changed, 219 insertions(+)
--
1.8.3.1
^ permalink raw reply
* [RFC PATCH 1/2] net: implement threaded-able napi poll loop support
From: Paolo Abeni @ 2016-05-10 14:11 UTC (permalink / raw)
To: netdev
Cc: 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>
This patch allows running each napi poll loop inside its
own kernel thread.
The rx mode can be enabled per napi instance via the
newly addded napi_set_threaded() api; the requested kthread
will be created on demand and shut down on device stop.
Once that threaded mode is enabled and the kthread is
started, napi_schedule() will wake-up such thread instead
of scheduling the softirq.
The threaded poll loop behaves quite likely the net_rx_action,
but it does not have to manipulate local irqs and uses
an explicit scheduling point based on netdev_budget.
Signed-off-by: Paolo Abeni <pabeni@redhat.com>
Signed-off-by: Hannes Frederic Sowa <hannes@stressinduktion.org>
---
include/linux/netdevice.h | 4 ++
net/core/dev.c | 113 ++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 117 insertions(+)
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 63580e6..0722ed5 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -323,6 +323,7 @@ struct napi_struct {
struct list_head dev_list;
struct hlist_node napi_hash_node;
unsigned int napi_id;
+ struct task_struct *thread;
};
enum {
@@ -331,6 +332,7 @@ enum {
NAPI_STATE_NPSVC, /* Netpoll - don't dequeue from poll_list */
NAPI_STATE_HASHED, /* In NAPI hash (busy polling possible) */
NAPI_STATE_NO_BUSY_POLL,/* Do not add in napi_hash, no busy polling */
+ NAPI_STATE_THREADED, /* The poll is performed inside its own thread*/
};
enum gro_result {
@@ -475,6 +477,8 @@ static inline void napi_complete(struct napi_struct *n)
*/
void napi_hash_add(struct napi_struct *napi);
+int napi_set_threaded(struct napi_struct *n, bool threded);
+
/**
* napi_hash_del - remove a NAPI from global table
* @napi: NAPI context
diff --git a/net/core/dev.c b/net/core/dev.c
index c749033..0de286b 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -94,6 +94,7 @@
#include <linux/ethtool.h>
#include <linux/notifier.h>
#include <linux/skbuff.h>
+#include <linux/kthread.h>
#include <net/net_namespace.h>
#include <net/sock.h>
#include <net/busy_poll.h>
@@ -1305,9 +1306,19 @@ void netdev_notify_peers(struct net_device *dev)
}
EXPORT_SYMBOL(netdev_notify_peers);
+static int napi_threaded_poll(void *data);
+
+static inline void napi_thread_start(struct napi_struct *n)
+{
+ if (test_bit(NAPI_STATE_THREADED, &n->state) && !n->thread)
+ n->thread = kthread_create(napi_threaded_poll, n, "%s-%d",
+ n->dev->name, n->napi_id);
+}
+
static int __dev_open(struct net_device *dev)
{
const struct net_device_ops *ops = dev->netdev_ops;
+ struct napi_struct *n;
int ret;
ASSERT_RTNL();
@@ -1334,6 +1345,9 @@ static int __dev_open(struct net_device *dev)
if (!ret && ops->ndo_open)
ret = ops->ndo_open(dev);
+ list_for_each_entry(n, &dev->napi_list, dev_list)
+ napi_thread_start(n);
+
netpoll_poll_enable(dev);
if (ret)
@@ -1378,6 +1392,14 @@ int dev_open(struct net_device *dev)
}
EXPORT_SYMBOL(dev_open);
+static inline void napi_thread_stop(struct napi_struct *n)
+{
+ if (!n->thread)
+ return;
+ kthread_stop(n->thread);
+ n->thread = NULL;
+}
+
static int __dev_close_many(struct list_head *head)
{
struct net_device *dev;
@@ -1406,6 +1428,7 @@ static int __dev_close_many(struct list_head *head)
list_for_each_entry(dev, head, close_list) {
const struct net_device_ops *ops = dev->netdev_ops;
+ struct napi_struct *n;
/*
* Call the device specific close. This cannot fail.
@@ -1417,6 +1440,9 @@ static int __dev_close_many(struct list_head *head)
if (ops->ndo_stop)
ops->ndo_stop(dev);
+ list_for_each_entry(n, &dev->napi_list, dev_list)
+ napi_thread_stop(n);
+
dev->flags &= ~IFF_UP;
netpoll_poll_enable(dev);
}
@@ -3456,6 +3482,11 @@ int weight_p __read_mostly = 64; /* old backlog weight */
static inline void ____napi_schedule(struct softnet_data *sd,
struct napi_struct *napi)
{
+ if (napi->thread) {
+ wake_up_process(napi->thread);
+ return;
+ }
+
list_add_tail(&napi->poll_list, &sd->poll_list);
__raise_softirq_irqoff(NET_RX_SOFTIRQ);
}
@@ -5174,6 +5205,88 @@ out_unlock:
return work;
}
+static int napi_thread_wait(struct napi_struct *napi)
+{
+ set_current_state(TASK_INTERRUPTIBLE);
+
+ while (!kthread_should_stop() && !napi_disable_pending(napi)) {
+ if (test_bit(NAPI_STATE_SCHED, &napi->state)) {
+ __set_current_state(TASK_RUNNING);
+ return 0;
+ }
+
+ schedule();
+ set_current_state(TASK_INTERRUPTIBLE);
+ }
+ __set_current_state(TASK_RUNNING);
+ return -1;
+}
+
+static int napi_threaded_poll(void *data)
+{
+ struct napi_struct *napi = data;
+
+ while (!napi_thread_wait(napi)) {
+ struct list_head dummy_repoll;
+ int budget = netdev_budget;
+ unsigned long time_limit;
+ bool again = true;
+
+ INIT_LIST_HEAD(&dummy_repoll);
+ local_bh_disable();
+ time_limit = jiffies + 2;
+ do {
+ /* ensure that the poll list is not empty */
+ if (list_empty(&dummy_repoll))
+ list_add(&napi->poll_list, &dummy_repoll);
+
+ budget -= napi_poll(napi, &dummy_repoll);
+ if (unlikely(budget <= 0 ||
+ time_after_eq(jiffies, time_limit))) {
+ cond_resched_softirq();
+
+ /* refresh the budget */
+ budget = netdev_budget;
+ __kfree_skb_flush();
+ time_limit = jiffies + 2;
+ }
+
+ if (napi_disable_pending(napi))
+ again = false;
+ else if (!test_bit(NAPI_STATE_SCHED, &napi->state))
+ again = false;
+ } while (again);
+
+ __kfree_skb_flush();
+ local_bh_enable();
+ }
+ return 0;
+}
+
+int napi_set_threaded(struct napi_struct *n, bool threaded)
+{
+ ASSERT_RTNL();
+
+ if (n->dev->flags & IFF_UP)
+ return -EBUSY;
+
+ if (threaded == !!test_bit(NAPI_STATE_THREADED, &n->state))
+ return 0;
+ if (threaded)
+ set_bit(NAPI_STATE_THREADED, &n->state);
+ else
+ clear_bit(NAPI_STATE_THREADED, &n->state);
+
+ /* if the device is initializing, nothing todo */
+ if (test_bit(__LINK_STATE_START, &n->dev->state))
+ return 0;
+
+ napi_thread_stop(n);
+ napi_thread_start(n);
+ return 0;
+}
+EXPORT_SYMBOL(napi_set_threaded);
+
static void net_rx_action(struct softirq_action *h)
{
struct softnet_data *sd = this_cpu_ptr(&softnet_data);
--
1.8.3.1
^ permalink raw reply related
* [RFC PATCH 2/2] net: add sysfs attribute to control napi threaded mode
From: Paolo Abeni @ 2016-05-10 14:11 UTC (permalink / raw)
To: netdev
Cc: 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>
this patch addis a new sysfs attribute to the network
device class. Said attribute is a bitmask that allows controlling
the threaded mode for all the napi instances of the given
network device.
The threaded mode can be switched only if related network device
is down.
Signed-off-by: Paolo Abeni <pabeni@redhat.com>
Signed-off-by: Hannes Frederic Sowa <hannes@stressinduktion.org>
---
net/core/net-sysfs.c | 102 +++++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 102 insertions(+)
diff --git a/net/core/net-sysfs.c b/net/core/net-sysfs.c
index 2b3f76f..60bc768 100644
--- a/net/core/net-sysfs.c
+++ b/net/core/net-sysfs.c
@@ -489,6 +489,107 @@ static ssize_t phys_switch_id_show(struct device *dev,
}
static DEVICE_ATTR_RO(phys_switch_id);
+unsigned long *__alloc_thread_bitmap(struct net_device *netdev, int *bits)
+{
+ struct napi_struct *n;
+
+ *bits = 0;
+ list_for_each_entry(n, &netdev->napi_list, dev_list)
+ (*bits)++;
+
+ return kmalloc_array(BITS_TO_LONGS(*bits), sizeof(unsigned long),
+ GFP_ATOMIC | __GFP_ZERO);
+}
+
+static ssize_t threaded_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ struct net_device *netdev = to_net_dev(dev);
+ struct napi_struct *n;
+ unsigned long *bmap;
+ size_t count = 0;
+ int i, bits;
+
+ if (!rtnl_trylock())
+ return restart_syscall();
+
+ if (!dev_isalive(netdev))
+ goto unlock;
+
+ bmap = __alloc_thread_bitmap(netdev, &bits);
+ if (!bmap) {
+ count = -ENOMEM;
+ goto unlock;
+ }
+
+ i = 0;
+ list_for_each_entry(n, &netdev->napi_list, dev_list) {
+ if (test_bit(NAPI_STATE_THREADED, &n->state))
+ set_bit(i, bmap);
+ i++;
+ }
+
+ count = bitmap_print_to_pagebuf(true, buf, bmap, bits);
+ kfree(bmap);
+
+unlock:
+ rtnl_unlock();
+
+ return count;
+}
+
+static ssize_t threaded_store(struct device *dev,
+ struct device_attribute *attr,
+ const char *buf, size_t len)
+{
+ struct net_device *netdev = to_net_dev(dev);
+ struct napi_struct *n;
+ unsigned long *bmap;
+ int i, bits;
+ size_t ret;
+
+ if (!capable(CAP_NET_ADMIN))
+ return -EPERM;
+
+ if (!rtnl_trylock())
+ return restart_syscall();
+
+ if (!dev_isalive(netdev)) {
+ ret = len;
+ goto unlock;
+ }
+
+ if (netdev->flags & IFF_UP) {
+ ret = -EBUSY;
+ goto unlock;
+ }
+
+ bmap = __alloc_thread_bitmap(netdev, &bits);
+ if (!bmap) {
+ ret = -ENOMEM;
+ goto unlock;
+ }
+
+ ret = bitmap_parselist(buf, bmap, bits);
+ if (ret)
+ goto free_unlock;
+
+ i = 0;
+ list_for_each_entry(n, &netdev->napi_list, dev_list) {
+ napi_set_threaded(n, test_bit(i, bmap));
+ i++;
+ }
+ ret = len;
+
+free_unlock:
+ kfree(bmap);
+
+unlock:
+ rtnl_unlock();
+ return ret;
+}
+static DEVICE_ATTR_RW(threaded);
+
static struct attribute *net_class_attrs[] = {
&dev_attr_netdev_group.attr,
&dev_attr_type.attr,
@@ -517,6 +618,7 @@ static struct attribute *net_class_attrs[] = {
&dev_attr_phys_port_name.attr,
&dev_attr_phys_switch_id.attr,
&dev_attr_proto_down.attr,
+ &dev_attr_threaded.attr,
NULL,
};
ATTRIBUTE_GROUPS(net_class);
--
1.8.3.1
^ permalink raw reply related
* [PATCH net-next] sfc: allocate rx pages on the same node as the interrupt
From: Edward Cree @ 2016-05-10 14:21 UTC (permalink / raw)
To: linux-net-drivers, David Miller, netdev; +Cc: Daniel Pieczko
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.
Allocating rx pages based on the channel->irq_node value is only
valid for the initial driver-load interrupt affinities; if an
interrupt is moved later, the wrong node may be used for the
allocation.
Signed-off-by: Bert Kenward <bkenward@solarflare.com>
Signed-off-by: Edward Cree <ecree@solarflare.com>
---
drivers/net/ethernet/sfc/efx.c | 38 +++++++++++++++++++++++++++++++++++
drivers/net/ethernet/sfc/net_driver.h | 3 +++
drivers/net/ethernet/sfc/rx.c | 18 ++++++++++++++---
3 files changed, 56 insertions(+), 3 deletions(-)
diff --git a/drivers/net/ethernet/sfc/efx.c b/drivers/net/ethernet/sfc/efx.c
index 0705ec86..cb1552c 100644
--- a/drivers/net/ethernet/sfc/efx.c
+++ b/drivers/net/ethernet/sfc/efx.c
@@ -445,6 +445,7 @@ efx_alloc_channel(struct efx_nic *efx, int i, struct efx_channel *old_channel)
channel->efx = efx;
channel->channel = i;
channel->type = &efx_default_channel_type;
+ channel->irq_mem_node = NUMA_NO_NODE;
for (j = 0; j < EFX_TXQ_TYPES; j++) {
tx_queue = &channel->tx_queue[j];
@@ -1486,6 +1487,38 @@ static int efx_probe_interrupts(struct efx_nic *efx)
return 0;
}
+#ifndef CONFIG_SMP
+static void efx_set_interrupt_affinity(struct efx_nic *efx __always_unused)
+{
+}
+
+static void efx_clear_interrupt_affinity(struct efx_nic *efx __always_unused)
+{
+}
+#else
+static void efx_set_interrupt_affinity(struct efx_nic *efx)
+{
+ struct efx_channel *channel;
+ unsigned int cpu;
+
+ efx_for_each_channel(channel, efx) {
+ cpu = cpumask_local_spread(channel->channel,
+ pcibus_to_node(efx->pci_dev->bus));
+
+ irq_set_affinity_hint(channel->irq, cpumask_of(cpu));
+ channel->irq_mem_node = cpu_to_mem(cpu);
+ }
+}
+
+static void efx_clear_interrupt_affinity(struct efx_nic *efx)
+{
+ struct efx_channel *channel;
+
+ efx_for_each_channel(channel, efx)
+ irq_set_affinity_hint(channel->irq, NULL);
+}
+#endif /* CONFIG_SMP */
+
static int efx_soft_enable_interrupts(struct efx_nic *efx)
{
struct efx_channel *channel, *end_channel;
@@ -2934,6 +2967,7 @@ static void efx_pci_remove_main(struct efx_nic *efx)
cancel_work_sync(&efx->reset_work);
efx_disable_interrupts(efx);
+ efx_clear_interrupt_affinity(efx);
efx_nic_fini_interrupt(efx);
efx_fini_port(efx);
efx->type->fini(efx);
@@ -3083,6 +3117,9 @@ static int efx_pci_probe_main(struct efx_nic *efx)
rc = efx_nic_init_interrupt(efx);
if (rc)
goto fail5;
+
+ efx_set_interrupt_affinity(efx);
+
rc = efx_enable_interrupts(efx);
if (rc)
goto fail6;
@@ -3090,6 +3127,7 @@ static int efx_pci_probe_main(struct efx_nic *efx)
return 0;
fail6:
+ efx_clear_interrupt_affinity(efx);
efx_nic_fini_interrupt(efx);
fail5:
efx_fini_port(efx);
diff --git a/drivers/net/ethernet/sfc/net_driver.h b/drivers/net/ethernet/sfc/net_driver.h
index 38c4223..28c3673 100644
--- a/drivers/net/ethernet/sfc/net_driver.h
+++ b/drivers/net/ethernet/sfc/net_driver.h
@@ -423,6 +423,7 @@ enum efx_sync_events_state {
* @sync_events_state: Current state of sync events on this channel
* @sync_timestamp_major: Major part of the last ptp sync event
* @sync_timestamp_minor: Minor part of the last ptp sync event
+ * @irq_mem_node: Memory NUMA node of interrupt
*/
struct efx_channel {
struct efx_nic *efx;
@@ -468,6 +469,8 @@ struct efx_channel {
enum efx_sync_events_state sync_events_state;
u32 sync_timestamp_major;
u32 sync_timestamp_minor;
+
+ int irq_mem_node;
};
#ifdef CONFIG_NET_RX_BUSY_POLL
diff --git a/drivers/net/ethernet/sfc/rx.c b/drivers/net/ethernet/sfc/rx.c
index 8956995..eed0c3b 100644
--- a/drivers/net/ethernet/sfc/rx.c
+++ b/drivers/net/ethernet/sfc/rx.c
@@ -163,9 +163,21 @@ static int efx_init_rx_buffers(struct efx_rx_queue *rx_queue, bool atomic)
do {
page = efx_reuse_page(rx_queue);
if (page == NULL) {
- page = alloc_pages(__GFP_COLD | __GFP_COMP |
- (atomic ? GFP_ATOMIC : GFP_KERNEL),
- efx->rx_buffer_order);
+ /* GFP_ATOMIC may fail because of various reasons,
+ * and we re-schedule rx_fill from non-atomic
+ * context in such a case. So, use __GFP_NO_WARN
+ * in case of atomic.
+ */
+ struct efx_channel *channel;
+
+ channel = efx_rx_queue_channel(rx_queue);
+ page = alloc_pages_node(channel->irq_mem_node,
+ __GFP_COMP |
+ (atomic ?
+ (GFP_ATOMIC | __GFP_NOWARN)
+ : GFP_KERNEL),
+ efx->rx_buffer_order);
+
if (unlikely(page == NULL))
return -ENOMEM;
dma_addr =
^ permalink raw reply related
* Re: [RFC PATCH 0/2] net: threadable napi poll loop
From: Eric Dumazet @ 2016-05-10 14:29 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, 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 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.
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.
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.
So you are shifting the attack to a different target,
at the expense of more kernel bloat.
^ permalink raw reply
* 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
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