* Re: [PATCH net 2/2] xdp: disallow use of native and generic hook at once
From: Jakub Kicinski @ 2017-05-10 21:07 UTC (permalink / raw)
To: Daniel Borkmann; +Cc: davem, alexei.starovoitov, john.fastabend, netdev
In-Reply-To: <5912DF16.7050603@iogearbox.net>
On Wed, 10 May 2017 11:36:22 +0200, Daniel Borkmann wrote:
> On 05/10/2017 05:18 AM, Jakub Kicinski wrote:
> > On Wed, 10 May 2017 03:31:31 +0200, Daniel Borkmann wrote:
> >> @@ -6851,6 +6851,32 @@ int dev_change_proto_down(struct net_device *dev, bool proto_down)
> >> }
> >> EXPORT_SYMBOL(dev_change_proto_down);
> >>
> >> +bool __dev_xdp_attached(struct net_device *dev, xdp_op_t xdp_op)
> >
> > Out of curiosity - the leading underscores refer to caller having to
> > hold rtnl? I assume they are not needed in the function below because
> > it's static?
>
> I think I don't quite follow the last question, but it probably makes
> sense to add an ASSERT_RTNL() into dev_xdp_attached() inline helper to
> make it clearly visible to callers of this api.
Sorry, I missed you have a dev_xdp_attached() defined in the header,
hence the confusion.
> >> +{
> >> + struct netdev_xdp xdp;
> >> +
> >> + memset(&xdp, 0, sizeof(xdp));
> >> + xdp.command = XDP_QUERY_PROG;
> >
> > Probably personal preference, but seems like designated struct
> > initializer would do quite nicely here and save the memset :)
>
> I had that initially, but I recalled that gcc < 4.6 does not handle this
> style for the initialization of anonymous struct/union properly (e.g.,
> we fixed that in iproute2 as well). Andrew Morton still uses gcc 4.4.4
> and occasionally sends kernel fixes, so we might end up like this anyway.
Ah, good to know!
> >> diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
> >> index dda9f16..99320f0 100644
> >> --- a/net/core/rtnetlink.c
> >> +++ b/net/core/rtnetlink.c
> >> @@ -1251,24 +1251,20 @@ static int rtnl_xdp_fill(struct sk_buff *skb, struct net_device *dev)
> >> {
> >> struct nlattr *xdp;
> >> u32 xdp_flags = 0;
> >> - u8 val = 0;
> >> int err;
> >> + u8 val;
> >>
> >> xdp = nla_nest_start(skb, IFLA_XDP);
> >> if (!xdp)
> >> return -EMSGSIZE;
> >> +
> >> if (rcu_access_pointer(dev->xdp_prog)) {
> >> xdp_flags = XDP_FLAGS_SKB_MODE;
> >> val = 1;
> >> - } else if (dev->netdev_ops->ndo_xdp) {
> >> - struct netdev_xdp xdp_op = {};
> >> -
> >> - xdp_op.command = XDP_QUERY_PROG;
> >> - err = dev->netdev_ops->ndo_xdp(dev, &xdp_op);
> >> - if (err)
> >> - goto err_cancel;
> >> - val = xdp_op.prog_attached;
> >> + } else {
> >> + val = dev_xdp_attached(dev);
> >> }
> >
> > Would it make sense to set xdp_flags to XDP_FLAGS_DRV_MODE here to keep
> > things symmetrical? I know you are just preserving existing behaviour
> > but it may seem slightly arbitrary to a new comer to report one of the
> > very similarly named flags in the dump but not the other.
>
> I thought about it, it's kind of redundant information since
> IFLA_XDP_ATTACHED attribute w/o IFLA_XDP_FLAGS attribute today
> says that it's native already. It might look strange if we add
> also XDP_FLAGS_DRV_MODE there, since it doesn't give anything
> new. I rather see it similar to XDP_FLAGS_UPDATE_IF_NOEXIST flag
> that is for updating fd only, but I don't really have a strong
> opinion on this though. I could add it to the respin if preferred.
XDP_FLAGS_UPDATE_IF_NOEXIST is indeed the precedent which makes things
a bit murky. There are no reasonably useful semantics for IF_NOEXIST
on dump :( Note that meaning of SKB_MODE flag shifts slightly between
set and dump IIUC. At set time it means:
"force installation at the generic hook",
at dump time it means:
"installed at generic hook - regardless of whether the flag was set at
installation time",
So I would argue that DRV_MODE flag is closer to SKB_MODE not only by
name but also by semantics, and it would be cool if we could keep the
semantics close on dump as well as set.
I understand the counter argument that from user space perspective it
would make things slightly more complicated because there would be two
conditions in which driver hook is used:
1) DRV_MODE set on dump;
2) flags attribute not present (old kernel).
I'm concerned about number 2). We can't simply depend on SKB_MODE
not being set because we may add more *_MODE flags in the future. So
doing:
if (flags & SKB_MODE)
printf("skb-mode");
else
printf("drv-mode");
is not correct. The flags attribute must not be present at all (think
HW_MODE flag). But going further there can also be non-MODE flags,
like, say.. NEVER_TX, and then there may be flags present in dump,
and if SKB_MODE isn't be set, the mode could be some new MODE user space
doesn't understand, or it could be DRV_MODE+a new non-MODE flag... no
way to tell :S
^ permalink raw reply
* Re: Hello
From: Rechel Diarra @ 2017-05-10 21:25 UTC (permalink / raw)
--
Hello, I have something important to discuss with you as soon as you reply back.
Regards.
Miss.Rechel
^ permalink raw reply
* [PATCH net-next] ipv6: Implement limits on hop by hop and destination options
From: Tom Herbert @ 2017-05-10 21:33 UTC (permalink / raw)
To: netdev; +Cc: kafai, Tom Herbert
RFC 2460 (IPv6) defines hop by hop options and destination options
extension headers. Both of these carry a list of TLVs which is
only limited by the maximum length of the extension header (2048
bytes). By the spec a host must process all the TLVs in these
options, however these could be used as a fairly obvious
denial of service attack. I think this could in fact be
a significant DOS vector on the Internet, one mitigating
factor might be that many FWs drop all packets with EH (and
obviously this is only IPv6) so an Internet wide attack might not
be so effective (yet!).
By my calculation, the worse case packet with TLVs in a standard
1500 byte MTU packet that would be processed by the stack contains
1282 invidual TLVs (including pad TLVS) or 724 two byte TLVs. I
wrote a quick test program that floods a whole bunch of these
packets to a host and sure enough there is substantial time spent
in ip6_parse_tlv. These packets contain nothing but unknown TLVS
(that are ignored), TLV padding, and bogus UDP header with zero
payload length.
25.38% [kernel] [k] __fib6_clean_all
21.63% [kernel] [k] ip6_parse_tlv
4.21% [kernel] [k] __local_bh_enable_ip
2.18% [kernel] [k] ip6_pol_route.isra.39
1.98% [kernel] [k] fib6_walk_continue
1.88% [kernel] [k] _raw_write_lock_bh
1.65% [kernel] [k] dst_release
This patches adds configurable limits to destination and hop by hop
options. There are three limits that may be set:
- Limit the number of non-padding TLVs that may be in an extension header
- Limit the length of a hop by hop or destination options extension header
- Disallow unknown options
The limits are set in corresponding sysctls:
ipv6.sysctl.max_dst_opts_cnt
ipv6.sysctl.max_hbh_opts_cnt
ipv6.sysctl.max_dst_opts_len
ipv6.sysctl.max_hbh_opts_len
If a max_*_opts_cnt is less than zero then unknown TLVs are disallowed.
The number of known TLVs that are allowed is the absolute value of
this number.
If a limit is exceeded when processing an extension header the packet is
dropped.
Default values are set to 8 for options counts, and set to INT_MAX
for maximum length. Note the choice to limit options to 8 is an
arbitrary guess (roughly based on the fact that the stack supports
three HBH options and just one destination option).
Tested: I've only complied this code, working on getting a test
environment set up which is why RFC. If anyone has resources and time
to do some testing or development, let me know!
Tested:
Martin Lau was kind enough to test this on real HW. He verified
that processing IPv6 TLVs CPU intensive and could very well be a
DDOS, and he verified that the patch drops packets that exceed the
limits to avoid the issue:
I tested out 1 thread (i.e. one raw_udp process).
I changed the net.ipv6.max_dst_(opts|hbh)_number between 8 to 2048.
With sysctls setting to 2048, the softirq% is packed to 100%.
With 8, the softirq% is almost unnoticable from mpstat.
---
Documentation/networking/ip-sysctl.txt | 22 +++++++++++++++++
include/net/ipv6.h | 33 +++++++++++++++++++++++++
include/net/netns/ipv6.h | 4 ++++
net/ipv6/af_inet6.c | 4 ++++
net/ipv6/exthdrs.c | 44 ++++++++++++++++++++++++++++++----
net/ipv6/sysctl_net_ipv6.c | 32 +++++++++++++++++++++++++
6 files changed, 134 insertions(+), 5 deletions(-)
diff --git a/Documentation/networking/ip-sysctl.txt b/Documentation/networking/ip-sysctl.txt
index 974ab47..476a5c5 100644
--- a/Documentation/networking/ip-sysctl.txt
+++ b/Documentation/networking/ip-sysctl.txt
@@ -1379,6 +1379,28 @@ mld_qrv - INTEGER
Default: 2 (as specified by RFC3810 9.1)
Minimum: 1 (as specified by RFC6636 4.5)
+max_dst_opts_cnt - INTEGER
+ Maximum number of non-padding TLVs allowed in a destination
+ options extension header. If this value is less than zero
+ then unknown options are disallowed and the number of known
+ TLVs allowed are the absolute value of this numer.
+
+ Default: 8
+
+max_hbh_opts_cnt - INTEGER
+ Maximum number of non-padding TLVs allowed in a hop by hop
+ options extension header. If this value is less than zero
+ then unknown options are disallowed and the number of known
+ TLVs allowed are the absolute value of this number.
+
+max dst_opts_len - INTEGER
+ Maximum length allowed for a destination options extension
+ header.
+
+max hbh_opts_len - INTEGER
+ Maximum length allowed for a hop by hop options extension
+ header.
+
IPv6 Fragmentation:
ip6frag_high_thresh - INTEGER
diff --git a/include/net/ipv6.h b/include/net/ipv6.h
index dbf0abb..9f724ae 100644
--- a/include/net/ipv6.h
+++ b/include/net/ipv6.h
@@ -50,6 +50,39 @@
#define IPV6_DEFAULT_HOPLIMIT 64
#define IPV6_DEFAULT_MCASTHOPS 1
+/* Limits on hop by hop and destination options.
+ *
+ * Per RFC2640 there is no limit on the maximum number or lengths of TLVs in
+ * hop by hop or destination options other then the packet must fit in an MTU.
+ * We allow configurable limits in order to mitigate potential denial of
+ * service attacks.
+ *
+ * There are three limits that may be set:
+ * - Limit the number of non-padding TLVs that may be in an extension header
+ * - Limit the length of a hop by hop or destination options extension header
+ * - Disallow unknown options
+ *
+ * The limits are set in corresponding sysctls:
+ *
+ * ipv6.sysctl.max_dst_opts_cnt
+ * ipv6.sysctl.max_hbh_opts_cnt
+ * ipv6.sysctl.max_dst_opts_len
+ * ipv6.sysctl.max_hbh_opts_len
+ *
+ * If a max_*_opts_cnt is less than zero then unknown TLVs are disallowed.
+ * The number of known TLVs that are allowed is the absolute value of
+ * this number.
+ *
+ * If a limit is exceeded when processing an extension header the packet is
+ * dropped.
+ */
+
+/* Default limits for hop by hop and destination options */
+#define IP6_DEFAULT_MAX_DST_OPTS_CNT 8
+#define IP6_DEFAULT_MAX_HBH_OPTS_CNT 8
+#define IP6_DEFAULT_MAX_DST_OPTS_LEN INT_MAX /* No limit */
+#define IP6_DEFAULT_MAX_HBH_OPTS_LEN INT_MAX /* No limit */
+
/*
* Addr type
*
diff --git a/include/net/netns/ipv6.h b/include/net/netns/ipv6.h
index de7745e..655bd236 100644
--- a/include/net/netns/ipv6.h
+++ b/include/net/netns/ipv6.h
@@ -36,6 +36,10 @@ struct netns_sysctl_ipv6 {
int idgen_retries;
int idgen_delay;
int flowlabel_state_ranges;
+ int max_dst_opts_cnt;
+ int max_hbh_opts_cnt;
+ int max_dst_opts_len;
+ int max_hbh_opts_len;
};
struct netns_ipv6 {
diff --git a/net/ipv6/af_inet6.c b/net/ipv6/af_inet6.c
index a88b5b5..38e1079 100644
--- a/net/ipv6/af_inet6.c
+++ b/net/ipv6/af_inet6.c
@@ -807,6 +807,10 @@ static int __net_init inet6_net_init(struct net *net)
net->ipv6.sysctl.idgen_retries = 3;
net->ipv6.sysctl.idgen_delay = 1 * HZ;
net->ipv6.sysctl.flowlabel_state_ranges = 0;
+ net->ipv6.sysctl.max_dst_opts_cnt = IP6_DEFAULT_MAX_DST_OPTS_CNT;
+ net->ipv6.sysctl.max_hbh_opts_cnt = IP6_DEFAULT_MAX_HBH_OPTS_CNT;
+ net->ipv6.sysctl.max_dst_opts_len = IP6_DEFAULT_MAX_DST_OPTS_LEN;
+ net->ipv6.sysctl.max_hbh_opts_len = IP6_DEFAULT_MAX_HBH_OPTS_LEN;
atomic_set(&net->ipv6.fib6_sernum, 1);
err = ipv6_init_mibs(net);
diff --git a/net/ipv6/exthdrs.c b/net/ipv6/exthdrs.c
index b636f1da..9cb09e1 100644
--- a/net/ipv6/exthdrs.c
+++ b/net/ipv6/exthdrs.c
@@ -100,13 +100,22 @@ static bool ip6_tlvopt_unknown(struct sk_buff *skb, int optoff)
/* Parse tlv encoded option header (hop-by-hop or destination) */
-static bool ip6_parse_tlv(const struct tlvtype_proc *procs, struct sk_buff *skb)
+static bool ip6_parse_tlv(const struct tlvtype_proc *procs,
+ struct sk_buff *skb,
+ int max_count)
{
const struct tlvtype_proc *curr;
const unsigned char *nh = skb_network_header(skb);
int off = skb_network_header_len(skb);
int len = (skb_transport_header(skb)[1] + 1) << 3;
int padlen = 0;
+ int tlv_count = 0;
+ bool disallow_unknowns = false;
+
+ if (unlikely(max_count < 0)) {
+ disallow_unknowns = true;
+ max_count = -max_count;
+ }
if (skb_transport_offset(skb) + len > skb_headlen(skb))
goto bad;
@@ -148,6 +157,11 @@ static bool ip6_parse_tlv(const struct tlvtype_proc *procs, struct sk_buff *skb)
default: /* Other TLV code so scan list */
if (optlen > len)
goto bad;
+
+ tlv_count++;
+ if (tlv_count > max_count)
+ goto bad;
+
for (curr = procs; curr->type >= 0; curr++) {
if (curr->type == nh[off]) {
/* type specific length/alignment
@@ -161,7 +175,10 @@ static bool ip6_parse_tlv(const struct tlvtype_proc *procs, struct sk_buff *skb)
if (curr->type < 0) {
if (ip6_tlvopt_unknown(skb, off) == 0)
return false;
+ if (disallow_unknowns)
+ goto bad;
}
+
padlen = 0;
break;
}
@@ -260,23 +277,31 @@ static int ipv6_destopt_rcv(struct sk_buff *skb)
__u16 dstbuf;
#endif
struct dst_entry *dst = skb_dst(skb);
+ struct net *net = dev_net(skb->dev);
+ int extlen;
if (!pskb_may_pull(skb, skb_transport_offset(skb) + 8) ||
!pskb_may_pull(skb, (skb_transport_offset(skb) +
((skb_transport_header(skb)[1] + 1) << 3)))) {
+fail_and_free:
__IP6_INC_STATS(dev_net(dst->dev), ip6_dst_idev(dst),
IPSTATS_MIB_INHDRERRORS);
kfree_skb(skb);
return -1;
}
+ extlen = (skb_transport_header(skb)[1] + 1) << 3;
+ if (extlen > net->ipv6.sysctl.max_dst_opts_len)
+ goto fail_and_free;
+
opt->lastopt = opt->dst1 = skb_network_header_len(skb);
#if IS_ENABLED(CONFIG_IPV6_MIP6)
dstbuf = opt->dst1;
#endif
- if (ip6_parse_tlv(tlvprocdestopt_lst, skb)) {
- skb->transport_header += (skb_transport_header(skb)[1] + 1) << 3;
+ if (ip6_parse_tlv(tlvprocdestopt_lst, skb,
+ init_net.ipv6.sysctl.max_dst_opts_cnt)) {
+ skb->transport_header += extlen;
opt = IP6CB(skb);
#if IS_ENABLED(CONFIG_IPV6_MIP6)
opt->nhoff = dstbuf;
@@ -804,6 +829,8 @@ static const struct tlvtype_proc tlvprochopopt_lst[] = {
int ipv6_parse_hopopts(struct sk_buff *skb)
{
struct inet6_skb_parm *opt = IP6CB(skb);
+ struct net *net = dev_net(skb->dev);
+ int extlen;
/*
* skb_network_header(skb) is equal to skb->data, and
@@ -818,9 +845,16 @@ int ipv6_parse_hopopts(struct sk_buff *skb)
return -1;
}
+ extlen = (skb_transport_header(skb)[1] + 1) << 3;
+ if (extlen > net->ipv6.sysctl.max_dst_opts_len) {
+ kfree_skb(skb);
+ return -1;
+ }
+
opt->flags |= IP6SKB_HOPBYHOP;
- if (ip6_parse_tlv(tlvprochopopt_lst, skb)) {
- skb->transport_header += (skb_transport_header(skb)[1] + 1) << 3;
+ if (ip6_parse_tlv(tlvprochopopt_lst, skb,
+ init_net.ipv6.sysctl.max_hbh_opts_cnt)) {
+ skb->transport_header += extlen;
opt = IP6CB(skb);
opt->nhoff = sizeof(struct ipv6hdr);
return 1;
diff --git a/net/ipv6/sysctl_net_ipv6.c b/net/ipv6/sysctl_net_ipv6.c
index 69c50e7..054cabe 100644
--- a/net/ipv6/sysctl_net_ipv6.c
+++ b/net/ipv6/sysctl_net_ipv6.c
@@ -90,6 +90,34 @@ static struct ctl_table ipv6_table_template[] = {
.mode = 0644,
.proc_handler = proc_dointvec
},
+ {
+ .procname = "max_dst_opts_number",
+ .data = &init_net.ipv6.sysctl.max_dst_opts_cnt,
+ .maxlen = sizeof(int),
+ .mode = 0644,
+ .proc_handler = proc_dointvec
+ },
+ {
+ .procname = "max_hbh_opts_number",
+ .data = &init_net.ipv6.sysctl.max_hbh_opts_cnt,
+ .maxlen = sizeof(int),
+ .mode = 0644,
+ .proc_handler = proc_dointvec
+ },
+ {
+ .procname = "max_dst_opts_length",
+ .data = &init_net.ipv6.sysctl.max_dst_opts_len,
+ .maxlen = sizeof(int),
+ .mode = 0644,
+ .proc_handler = proc_dointvec
+ },
+ {
+ .procname = "max_hbh_length",
+ .data = &init_net.ipv6.sysctl.max_hbh_opts_len,
+ .maxlen = sizeof(int),
+ .mode = 0644,
+ .proc_handler = proc_dointvec
+ },
{ }
};
@@ -149,6 +177,10 @@ static int __net_init ipv6_sysctl_net_init(struct net *net)
ipv6_table[6].data = &net->ipv6.sysctl.idgen_delay;
ipv6_table[7].data = &net->ipv6.sysctl.flowlabel_state_ranges;
ipv6_table[8].data = &net->ipv6.sysctl.ip_nonlocal_bind;
+ ipv6_table[9].data = &net->ipv6.sysctl.max_dst_opts_cnt;
+ ipv6_table[10].data = &net->ipv6.sysctl.max_hbh_opts_cnt;
+ ipv6_table[11].data = &net->ipv6.sysctl.max_dst_opts_len;
+ ipv6_table[12].data = &net->ipv6.sysctl.max_hbh_opts_len;
ipv6_route_table = ipv6_route_sysctl_init(net);
if (!ipv6_route_table)
--
2.7.4
^ permalink raw reply related
* Re: Requirements for a shutdown function?
From: Florian Fainelli @ 2017-05-10 21:47 UTC (permalink / raw)
To: Timur Tabi, netdev
In-Reply-To: <1efae594-427b-58e5-fd48-bac9f686bb52@codeaurora.org>
On 05/10/2017 01:17 PM, Timur Tabi wrote:
> On 05/09/2017 02:06 PM, Florian Fainelli wrote:
>> On 05/09/2017 11:51 AM, Timur Tabi wrote:
>
>>> Is it possible that the network stack detects a kexec and automatically
>>> stops all network devices?
>>
>> No. why would it? However the device driver model does call into your
>> driver's remove function and that one does a right job already because
>> it does an network device unregister, and so on.
>
> I ran some more tests. When I launch kexec, the driver's
> net_device_ops.ndo_stop function is called, which already stops the interface.
>
> So it looks to me as if the network stack does close the interface during a
> kexec. With the interface closed, there's no point in having a shutdown
> function, is there?
AFAIR kexec takes care of shutting down network devices explicitly
(unless instructed otherwise with -x/--no-ifdown) so this may be where
this is coming from.
Reading through drivers/base/core.c it does not appear that ->remove()
is called and then ->shutdown() gets called, only ->shutdown() gets
called from device_shutdown() called from kernel/reboot.c. It seems to
me like if you want to be on the safe side you would want to implement a
shutdown function that is identical to what your remove function does.
>
>>> My in-house driver stops the RX and TX queues. I'm guessing that's good
>>> enough, but I don't have a failing test case to prove it.
>>>
>>
>> That's probably good enough, yes.
>
> Except that it turns out that the queues are already stopped by then because
> the emac_close() function has already been called.
--
Florian
^ permalink raw reply
* Re: [PATCH v2 net-next 03/12] drivers: net: xgene: Use rgmii mdio mac access
From: Florian Fainelli @ 2017-05-10 21:48 UTC (permalink / raw)
To: Iyappan Subramanian, davem, netdev; +Cc: Quan Nguyen, patches, linux-arm-kernel
In-Reply-To: <1494449110-23785-4-git-send-email-isubramanian@apm.com>
On 05/10/2017 01:45 PM, Iyappan Subramanian wrote:
> From: Quan Nguyen <qnguyen@apm.com>
>
> This patch switches to use rgmii mdio mac access routines if available,
> as they share the same HW.
>
> Signed-off-by: Quan Nguyen <qnguyen@apm.com>
> Signed-off-by: Iyappan Subramanian <isubramanian@apm.com>
> ---
> drivers/net/ethernet/apm/xgene/xgene_enet_hw.c | 14 ++++++++++++++
> 1 file changed, 14 insertions(+)
>
> diff --git a/drivers/net/ethernet/apm/xgene/xgene_enet_hw.c b/drivers/net/ethernet/apm/xgene/xgene_enet_hw.c
> index 2050c58..47c5b75 100644
> --- a/drivers/net/ethernet/apm/xgene/xgene_enet_hw.c
> +++ b/drivers/net/ethernet/apm/xgene/xgene_enet_hw.c
> @@ -277,6 +277,13 @@ void xgene_enet_wr_mac(struct xgene_enet_pdata *pdata, u32 wr_addr, u32 wr_data)
> u8 wait = 10;
> u32 done;
>
> + if (pdata->mdio_driver && ndev->phydev &&
> + pdata->phy_mode == PHY_INTERFACE_MODE_RGMII) {
To be on the safe side you should check all 4 values of
PHY_INTERFACE_MODE_RGMII, not just this one.
--
Florian
^ permalink raw reply
* Re: Requirements for a shutdown function?
From: Timur Tabi @ 2017-05-10 22:11 UTC (permalink / raw)
To: Florian Fainelli, netdev
In-Reply-To: <358a4a6f-5c47-dae8-38bf-67150cba1b07@gmail.com>
On 05/10/2017 04:47 PM, Florian Fainelli wrote:
> AFAIR kexec takes care of shutting down network devices explicitly
> (unless instructed otherwise with -x/--no-ifdown) so this may be where
> this is coming from.
>
> Reading through drivers/base/core.c it does not appear that ->remove()
> is called and then ->shutdown() gets called, only ->shutdown() gets
> called from device_shutdown() called from kernel/reboot.c. It seems to
> me like if you want to be on the safe side you would want to implement a
> shutdown function that is identical to what your remove function does.
I finally found a testcase where the shutdown function is useful. If you do
a "reboot -f", it will call shutdown but not close.
--
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc. Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.
^ permalink raw reply
* Re: Requirements for a shutdown function?
From: Florian Fainelli @ 2017-05-10 22:17 UTC (permalink / raw)
To: Timur Tabi, netdev
In-Reply-To: <54c18ee6-d748-59f7-fd6f-451559b86c0b@codeaurora.org>
On 05/10/2017 03:11 PM, Timur Tabi wrote:
> On 05/10/2017 04:47 PM, Florian Fainelli wrote:
>> AFAIR kexec takes care of shutting down network devices explicitly
>> (unless instructed otherwise with -x/--no-ifdown) so this may be where
>> this is coming from.
>>
>> Reading through drivers/base/core.c it does not appear that ->remove()
>> is called and then ->shutdown() gets called, only ->shutdown() gets
>> called from device_shutdown() called from kernel/reboot.c. It seems to
>> me like if you want to be on the safe side you would want to implement a
>> shutdown function that is identical to what your remove function does.
>
> I finally found a testcase where the shutdown function is useful. If you do
> a "reboot -f", it will call shutdown but not close.
Correct yes. Sorry, I did not recall which one of kexec or reboot would
call it, but both would actually now that I looked at what happens on
one of my systems again.
--
Florian
^ permalink raw reply
* Re: [PATCH net 2/2] xdp: disallow use of native and generic hook at once
From: Daniel Borkmann @ 2017-05-10 22:24 UTC (permalink / raw)
To: Jakub Kicinski; +Cc: davem, alexei.starovoitov, john.fastabend, netdev
In-Reply-To: <20170510140754.739e4123@cakuba.netronome.com>
On 05/10/2017 11:07 PM, Jakub Kicinski wrote:
> On Wed, 10 May 2017 11:36:22 +0200, Daniel Borkmann wrote:
>> On 05/10/2017 05:18 AM, Jakub Kicinski wrote:
>>> On Wed, 10 May 2017 03:31:31 +0200, Daniel Borkmann wrote:
[...]
>>>> xdp = nla_nest_start(skb, IFLA_XDP);
>>>> if (!xdp)
>>>> return -EMSGSIZE;
>>>> +
>>>> if (rcu_access_pointer(dev->xdp_prog)) {
>>>> xdp_flags = XDP_FLAGS_SKB_MODE;
>>>> val = 1;
>>>> - } else if (dev->netdev_ops->ndo_xdp) {
>>>> - struct netdev_xdp xdp_op = {};
>>>> -
>>>> - xdp_op.command = XDP_QUERY_PROG;
>>>> - err = dev->netdev_ops->ndo_xdp(dev, &xdp_op);
>>>> - if (err)
>>>> - goto err_cancel;
>>>> - val = xdp_op.prog_attached;
>>>> + } else {
>>>> + val = dev_xdp_attached(dev);
>>>> }
>>>
>>> Would it make sense to set xdp_flags to XDP_FLAGS_DRV_MODE here to keep
>>> things symmetrical? I know you are just preserving existing behaviour
>>> but it may seem slightly arbitrary to a new comer to report one of the
>>> very similarly named flags in the dump but not the other.
>>
>> I thought about it, it's kind of redundant information since
>> IFLA_XDP_ATTACHED attribute w/o IFLA_XDP_FLAGS attribute today
>> says that it's native already. It might look strange if we add
>> also XDP_FLAGS_DRV_MODE there, since it doesn't give anything
>> new. I rather see it similar to XDP_FLAGS_UPDATE_IF_NOEXIST flag
>> that is for updating fd only, but I don't really have a strong
>> opinion on this though. I could add it to the respin if preferred.
>
> XDP_FLAGS_UPDATE_IF_NOEXIST is indeed the precedent which makes things
> a bit murky. There are no reasonably useful semantics for IF_NOEXIST
> on dump :( Note that meaning of SKB_MODE flag shifts slightly between
> set and dump IIUC. At set time it means:
> "force installation at the generic hook",
> at dump time it means:
> "installed at generic hook - regardless of whether the flag was set at
> installation time",
>
> So I would argue that DRV_MODE flag is closer to SKB_MODE not only by
> name but also by semantics, and it would be cool if we could keep the
> semantics close on dump as well as set.
Right.
> I understand the counter argument that from user space perspective it
> would make things slightly more complicated because there would be two
> conditions in which driver hook is used:
> 1) DRV_MODE set on dump;
> 2) flags attribute not present (old kernel).
>
> I'm concerned about number 2). We can't simply depend on SKB_MODE
> not being set because we may add more *_MODE flags in the future. So
> doing:
>
> if (flags & SKB_MODE)
> printf("skb-mode");
> else
> printf("drv-mode");
>
> is not correct. The flags attribute must not be present at all (think
> HW_MODE flag). But going further there can also be non-MODE flags,
> like, say.. NEVER_TX, and then there may be flags present in dump,
> and if SKB_MODE isn't be set, the mode could be some new MODE user space
> doesn't understand, or it could be DRV_MODE+a new non-MODE flag... no
> way to tell :S
Yep, I see your point. Additionally, if we use XDP_FLAGS_* again for
dumping we're wasting bit space for flags we would never dump back
such as mentioned XDP_FLAGS_UPDATE_IF_NOEXIST (or any other future
flags that are only relevant for loading, but never for dumping).
Given dumping IFLA_XDP_FLAGS was added due to XDP_FLAGS_SKB_MODE,
we can still change this, since it's not too late.
How about the following proposal: IFLA_XDP_ATTACHED we have as-is
(need to keep that anyway), if that is true, it means "something
is attached at XDP layer". Then, we add a new attribute IFLA_XDP_MODE
(enum as type), which can contain XDP_DRV_MODE (0), XDP_SKB_MODE (1).
I don't think there's a strict requirement to really dump IFLA_XDP_FLAGS
back, separating both attrs would avoid this hassle of what current
or future load flag fits for dump as well and which not. Wdyt?
Thanks,
Daniel
^ permalink raw reply
* Re: [PATCH net 2/2] xdp: disallow use of native and generic hook at once
From: Jakub Kicinski @ 2017-05-10 22:46 UTC (permalink / raw)
To: Daniel Borkmann; +Cc: davem, alexei.starovoitov, john.fastabend, netdev
In-Reply-To: <59139338.5020405@iogearbox.net>
On Thu, 11 May 2017 00:24:56 +0200, Daniel Borkmann wrote:
> > I understand the counter argument that from user space perspective it
> > would make things slightly more complicated because there would be two
> > conditions in which driver hook is used:
> > 1) DRV_MODE set on dump;
> > 2) flags attribute not present (old kernel).
> >
> > I'm concerned about number 2). We can't simply depend on SKB_MODE
> > not being set because we may add more *_MODE flags in the future. So
> > doing:
> >
> > if (flags & SKB_MODE)
> > printf("skb-mode");
> > else
> > printf("drv-mode");
> >
> > is not correct. The flags attribute must not be present at all (think
> > HW_MODE flag). But going further there can also be non-MODE flags,
> > like, say.. NEVER_TX, and then there may be flags present in dump,
> > and if SKB_MODE isn't be set, the mode could be some new MODE user space
> > doesn't understand, or it could be DRV_MODE+a new non-MODE flag... no
> > way to tell :S
>
> Yep, I see your point. Additionally, if we use XDP_FLAGS_* again for
> dumping we're wasting bit space for flags we would never dump back
> such as mentioned XDP_FLAGS_UPDATE_IF_NOEXIST (or any other future
> flags that are only relevant for loading, but never for dumping).
> Given dumping IFLA_XDP_FLAGS was added due to XDP_FLAGS_SKB_MODE,
> we can still change this, since it's not too late.
>
> How about the following proposal: IFLA_XDP_ATTACHED we have as-is
> (need to keep that anyway), if that is true, it means "something
> is attached at XDP layer". Then, we add a new attribute IFLA_XDP_MODE
> (enum as type), which can contain XDP_DRV_MODE (0), XDP_SKB_MODE (1).
> I don't think there's a strict requirement to really dump IFLA_XDP_FLAGS
> back, separating both attrs would avoid this hassle of what current
> or future load flag fits for dump as well and which not. Wdyt?
I really like the idea of not reusing IFLA_XDP_FLAGS for dumps! New
enum sounds good, but perhaps reusing IFLA_XDP_ATTACHED isn't 100%
off-limits either? 4.11 would report (0) - driver supports XDP but
nothing is attached, (1) - something attached at the driver, could we
make (2) mean - something attached at generic XDP? Would that be
considered breaking the ABI, are we bound to boolean semantics?
^ permalink raw reply
* Re: [PATCH net 2/2] xdp: disallow use of native and generic hook at once
From: Daniel Borkmann @ 2017-05-10 23:02 UTC (permalink / raw)
To: Jakub Kicinski; +Cc: davem, alexei.starovoitov, john.fastabend, netdev
In-Reply-To: <20170510154630.316010f9@cakuba.netronome.com>
On 05/11/2017 12:46 AM, Jakub Kicinski wrote:
> On Thu, 11 May 2017 00:24:56 +0200, Daniel Borkmann wrote:
>>> I understand the counter argument that from user space perspective it
>>> would make things slightly more complicated because there would be two
>>> conditions in which driver hook is used:
>>> 1) DRV_MODE set on dump;
>>> 2) flags attribute not present (old kernel).
>>>
>>> I'm concerned about number 2). We can't simply depend on SKB_MODE
>>> not being set because we may add more *_MODE flags in the future. So
>>> doing:
>>>
>>> if (flags & SKB_MODE)
>>> printf("skb-mode");
>>> else
>>> printf("drv-mode");
>>>
>>> is not correct. The flags attribute must not be present at all (think
>>> HW_MODE flag). But going further there can also be non-MODE flags,
>>> like, say.. NEVER_TX, and then there may be flags present in dump,
>>> and if SKB_MODE isn't be set, the mode could be some new MODE user space
>>> doesn't understand, or it could be DRV_MODE+a new non-MODE flag... no
>>> way to tell :S
>>
>> Yep, I see your point. Additionally, if we use XDP_FLAGS_* again for
>> dumping we're wasting bit space for flags we would never dump back
>> such as mentioned XDP_FLAGS_UPDATE_IF_NOEXIST (or any other future
>> flags that are only relevant for loading, but never for dumping).
>> Given dumping IFLA_XDP_FLAGS was added due to XDP_FLAGS_SKB_MODE,
>> we can still change this, since it's not too late.
>>
>> How about the following proposal: IFLA_XDP_ATTACHED we have as-is
>> (need to keep that anyway), if that is true, it means "something
>> is attached at XDP layer". Then, we add a new attribute IFLA_XDP_MODE
>> (enum as type), which can contain XDP_DRV_MODE (0), XDP_SKB_MODE (1).
>> I don't think there's a strict requirement to really dump IFLA_XDP_FLAGS
>> back, separating both attrs would avoid this hassle of what current
>> or future load flag fits for dump as well and which not. Wdyt?
>
> I really like the idea of not reusing IFLA_XDP_FLAGS for dumps! New
> enum sounds good, but perhaps reusing IFLA_XDP_ATTACHED isn't 100%
> off-limits either? 4.11 would report (0) - driver supports XDP but
> nothing is attached, (1) - something attached at the driver, could we
> make (2) mean - something attached at generic XDP? Would that be
> considered breaking the ABI, are we bound to boolean semantics?
I like the idea, it would also render IFLA_XDP_ATTACHED not useless
or redundant then. Older binaries check !rta_getattr_u8(tb[IFLA_XDP_ATTACHED])
whether something is attached or not at XDP layer. So they won't know
IFLA_XDP_FLAGS attr either to make a more fine-grained distinction about
what mode. That seems actually cleaner to me, will give it a try.
Thanks,
Daniel
^ permalink raw reply
* Re: [PATCH] libertas: Avoid reading past end of buffer
From: Joe Perches @ 2017-05-10 23:05 UTC (permalink / raw)
To: Kees Cook, netdev
Cc: Kalle Valo, libertas-dev, linux-wireless, Daniel Micay,
linux-kernel
In-Reply-To: <20170510192451.GA115771@beast>
On Wed, 2017-05-10 at 12:24 -0700, Kees Cook wrote:
> Using memcpy() from a string that is shorter than the length copied means
[]
> diff --git a/drivers/net/wireless/marvell/libertas/mesh.c b/drivers/net/wireless/marvell/libertas/mesh.c
[]
> @@ -1170,17 +1170,11 @@ int lbs_mesh_ethtool_get_sset_count(struct net_device *dev, int sset)
> void lbs_mesh_ethtool_get_strings(struct net_device *dev,
> uint32_t stringset, uint8_t *s)
> {
> - int i;
> -
> lbs_deb_enter(LBS_DEB_ETHTOOL);
>
> switch (stringset) {
> case ETH_SS_STATS:
> - for (i = 0; i < MESH_STATS_NUM; i++) {
> - memcpy(s + i * ETH_GSTRING_LEN,
> - mesh_stat_strings[i],
> - ETH_GSTRING_LEN);
> - }
> + memcpy(s, *mesh_stat_strings, sizeof(mesh_stat_strings));
> break;
> }
> lbs_deb_enter(LBS_DEB_ETHTOOL);
unrelated trivia:
lbs_deb_enter is used incorrectly here at
function exit as both enter and leave calls.
That type of copy/paste defect may be common.
$ git grep -w lbs_deb_enter | wc -l
148
$ git grep -w lbs_deb_leave | wc -l
71
One would expect these numbers to be the same.
Another option would be to delete all these
calls as ftrace function tracing works well.
^ permalink raw reply
* Re: [PATCH] libertas: Avoid reading past end of buffer
From: Joe Perches @ 2017-05-10 23:12 UTC (permalink / raw)
To: Kees Cook, netdev-u79uwXL29TY76Z2rM5mHXA
Cc: Kalle Valo, libertas-dev-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-wireless-u79uwXL29TY76Z2rM5mHXA, Daniel Micay,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <20170510192451.GA115771@beast>
On Wed, 2017-05-10 at 12:24 -0700, Kees Cook wrote:
> Using memcpy() from a string that is shorter than the length copied means
> the destination buffer is being filled with arbitrary data from the kernel
> rodata segment.
another bit of trivia:
> diff --git a/drivers/net/wireless/marvell/libertas/mesh.c b/drivers/net/wireless/marvell/libertas/mesh.c
[]
> @@ -1170,17 +1170,11 @@ int lbs_mesh_ethtool_get_sset_count(struct net_device *dev, int sset)
[]
> + memcpy(s, *mesh_stat_strings, sizeof(mesh_stat_strings));
That * isn't necessary.
^ permalink raw reply
* [PATCH net] bpf, arm64: fix faulty emission of map access in tail calls
From: Daniel Borkmann @ 2017-05-10 23:53 UTC (permalink / raw)
To: davem
Cc: alexei.starovoitov, illusionist.neo, zlim.lnx, catalin.marinas,
netdev, linux-arm-kernel, Daniel Borkmann
Shubham was recently asking on netdev why in arm64 JIT we don't multiply
the index for accessing the tail call map by 8. That led me into testing
out arm64 JIT wrt tail calls and it turned out I got a NULL pointer
dereference on the tail call.
The buggy access is at:
prog = array->ptrs[index];
if (prog == NULL)
goto out;
[...]
00000060: d2800e0a mov x10, #0x70 // #112
00000064: f86a682a ldr x10, [x1,x10]
00000068: f862694b ldr x11, [x10,x2]
0000006c: b40000ab cbz x11, 0x00000080
[...]
The code triggering the crash is f862694b. x1 at the time contains the
address of the bpf array, x10 offsetof(struct bpf_array, ptrs). Meaning,
above we load the pointer to the program at map slot 0 into x10. x10
can then be NULL if the slot is not occupied, which we later on try to
access with a user given offset in x2 that is the map index.
Fix this by emitting the following instead:
[...]
00000060: d2800e0a mov x10, #0x70 // #112
00000064: 8b0a002a add x10, x1, x10
00000068: d37df04b lsl x11, x2, #3
0000006c: f86b694b ldr x11, [x10,x11]
00000070: b40000ab cbz x11, 0x00000084
[...]
This basically adds the offset to ptrs to the base address of the bpf
array we got and we later on access the map with an index * 8 offset
relative to that. The tail call map itself is basically one large area
with meta data at the head followed by the array of prog pointers.
This makes tail calls working again, tested on Cavium ThunderX ARMv8.
Fixes: ddb55992b04d ("arm64: bpf: implement bpf_tail_call() helper")
Reported-by: Shubham Bansal <illusionist.neo@gmail.com>
Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
---
arch/arm64/net/bpf_jit_comp.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/arch/arm64/net/bpf_jit_comp.c b/arch/arm64/net/bpf_jit_comp.c
index c6e5358..71f9305 100644
--- a/arch/arm64/net/bpf_jit_comp.c
+++ b/arch/arm64/net/bpf_jit_comp.c
@@ -253,8 +253,9 @@ static int emit_bpf_tail_call(struct jit_ctx *ctx)
*/
off = offsetof(struct bpf_array, ptrs);
emit_a64_mov_i64(tmp, off, ctx);
- emit(A64_LDR64(tmp, r2, tmp), ctx);
- emit(A64_LDR64(prg, tmp, r3), ctx);
+ emit(A64_ADD(1, tmp, r2, tmp), ctx);
+ emit(A64_LSL(1, prg, r3, 3), ctx);
+ emit(A64_LDR64(prg, tmp, prg), ctx);
emit(A64_CBZ(1, prg, jmp_offset), ctx);
/* goto *(prog->bpf_func + prologue_size); */
--
1.9.3
^ permalink raw reply related
* [PATCH net] tcp: avoid fragmenting peculiar skbs in SACK
From: Yuchung Cheng @ 2017-05-11 0:01 UTC (permalink / raw)
To: davem; +Cc: netdev, ncardwell, edumazet, soheil, Yuchung Cheng
This patch fixes a bug in splitting an SKB during SACK
processing. Specifically if an skb contains multiple
packets and is only partially sacked in the higher sequences,
tcp_match_sack_to_skb() splits the skb and marks the second fragment
as SACKed.
The current code further attempts rounding up the first fragment
to MSS boundaries. But it misses a boundary condition when the
rounded-up fragment size (pkt_len) is exactly skb size. Spliting
such an skb is pointless and causses a kernel warning and aborts
the SACK processing. This patch universally checks such over-split
before calling tcp_fragment to prevent these unnecessary warnings.
Fixes: adb92db857ee ("tcp: Make SACK code to split only at mss boundaries")
Signed-off-by: Yuchung Cheng <ycheng@google.com>
Signed-off-by: Eric Dumazet <edumazet@google.com>
Signed-off-by: Soheil Hassas Yeganeh <soheil@google.com>
---
net/ipv4/tcp_input.c | 9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
index 5a3ad09e2786..06e2dbc2b4a2 100644
--- a/net/ipv4/tcp_input.c
+++ b/net/ipv4/tcp_input.c
@@ -1179,13 +1179,14 @@ static int tcp_match_skb_to_sack(struct sock *sk, struct sk_buff *skb,
*/
if (pkt_len > mss) {
unsigned int new_len = (pkt_len / mss) * mss;
- if (!in_sack && new_len < pkt_len) {
+ if (!in_sack && new_len < pkt_len)
new_len += mss;
- if (new_len >= skb->len)
- return 0;
- }
pkt_len = new_len;
}
+
+ if (pkt_len >= skb->len && !in_sack)
+ return 0;
+
err = tcp_fragment(sk, skb, pkt_len, mss, GFP_ATOMIC);
if (err < 0)
return err;
--
2.13.0.rc2.291.g57267f2277-goog
^ permalink raw reply related
* Re: [PATCH 1/2] PCI: Add new PCIe Fabric End Node flag, PCI_DEV_FLAGS_NO_RELAXED_ORDERING
From: Ding Tianhong @ 2017-05-11 1:15 UTC (permalink / raw)
To: Casey Leedom, Alexander Duyck
Cc: Mark Rutland, Gabriele Paoloni, Asit K Mallick, Catalin Marinas,
Will Deacon, LinuxArm, Raj, Ashok, Bjorn Helgaas, Ganesh GR,
Jeff Kirsher, Bob Shaw, Patrick J Cramer, Arjun V.,
Michael Werner, linux-arm-kernel@lists.infradead.org, Amir Ancel,
Netdev, David Laight, Suravee Suthikulpanit, Robin Murphy,
David Miller, h
In-Reply-To: <MWHPR12MB1600AC957C7E3B0DB4F06D6CC8EE0@MWHPR12MB1600.namprd12.prod.outlook.com>
On 2017/5/9 8:48, Casey Leedom wrote:
>
> | From: Alexander Duyck <alexander.duyck@gmail.com>
> | Date: Saturday, May 6, 2017 11:07 AM
> |
> | | From: Ding Tianhong <dingtianhong@huawei.com>
> | | Date: Fri, May 5, 2017 at 8:08 PM
> | |
> | | According the suggestion, I could only think of this code:
> | | ..
> |
> | This is a bit simplistic but it is a start.
>
> Yes, something tells me that this is going to be more complicated than any
> of us like ...
>
> | The other bit I was getting at is that we need to update the core PCIe
> | code so that when we configure devices and the root complex reports no
> | support for relaxed ordering it should be clearing the relaxed
> | ordering bits in the PCIe configuration registers on the upstream
> | facing devices.
>
> Of course, this can be written to by the Driver at any time ... and is in
> the case of the cxgb4 Driver ...
>
> After a lot of rummaging around, it does look like KVM prohibits writes to
> the PCIe Capability Device Control register in drivers/xen/xen-pciback/
> conf_space_capability.c and conf_space.c simply because writes aren't
> allowed unless "permissive" is set. So it ~looks~ like a driver running in
> a Virtual Machine can't turn Enable Relaxed Ordering back on ...
>
> | The last bit we need in all this is a way to allow for setups where
> | peer-to-peer wants to perform relaxed ordering but for writes to the
> | host we have to not use relaxed ordering. For that we need to enable a
> | special case and that isn't handled right now in any of the solutions
> | we have coded up so far.
>
> Yes, we do need this.
>
>
> | From: Alexander Duyck <alexander.duyck@gmail.com>
> | Date: Saturday, May 8, 2017 08:22 AM
> |
> | The problem is we need to have something that can be communicated
> | through a VM. Your change doesn't work in that regard. That was why I
> | suggested just updating the code so that we when we initialized PCIe
> | devices what we do is either set or clear the relaxed ordering bit in
> | the PCIe device control register. That way when we direct assign an
> | interface it could know just based on the bits int the PCIe
> | configuration if it could use relaxed ordering or not.
> |
> | At that point the driver code itself becomes very simple since you
> | could just enable the relaxed ordering by default in the igb/ixgbe
> | driver and if the bit is set or cleared in the PCIe configuration then
> | we are either sending with relaxed ordering requests or not and don't
> | have to try and locate the root complex.
> |
> | So from the sound of it Casey has a special use case where he doesn't
> | want to send relaxed ordering frames to the root complex, but instead
> | would like to send them to another PCIe device. To do that he needs to
> | have a way to enable the relaxed ordering bit in the PCIe
> | configuration but then not send any to the root complex. Odds are that
> | is something he might be able to just implement in the driver, but is
> | something that may become a more general case in the future. I don't
> | see our change here impacting it as long as we keep the solution
> | generic and mostly confined to when we instantiate the devices as the
> | driver could likely make the decision to change the behavior later.
>
> It's not just me. Intel has said that while RO directed at the Root
> Complex Host Coherent Memory has a performance bug (not Data Corruption),
> it's a performance win for Peer-to-Peer writes to MMIO Space. (I'll be very
> interested in hearing what the bug is if we get that much detail. The very
> same TLPs directed to the Root Complex Port without Relaxed Ordering set get
> good performance. So this is essentially a bug in the hardware that was
> ~trying~ to implement a performance win.)
>
> Meanwhile, I currently only know of a single PCIe End Point which causes
> catastrophic results: the AMD A1100 ARM SoC ("SEATTLE"). And it's not even
> clear that product is even alive anymore since I haven't been able to get
> any responses from them for several months.
>
> What I'm saying is: let's try to architect a solution which doesn't throw
> the baby out with the bath water ...
>
> I think that if a Device's Root Complex Port has problems with Relaxed
> Ordering, it ~probably~ makes sense to turn off the PCIe Capability Device
> Control[Enable Relaxed Ordering] when we assign a device to a Virtual
> Machine since the Device Driver can no longer query the Relaxed Ordering
> Support of the Root Complex Port. The only down side of this would be if we
> assigned two Peers to a VM in an application which wanted to do Peer-to-Peer
> transfers. But that seems like a hard application to support in any case
> since the PCI Bus:Slot.Function IDs for assigned Devices within a VM don't
> match the actual values.
>
> For Devices running in the base OS/Hypervisor, their Drivers can query the
> Relaxed Ordering Support for the Root Complex Port or a Peer Device. So a
> simple flag within the (struct pci_dev *)->dev_flags would serve for that
> along with a per-Architecture/Platform mechanism for setting it ...
>
Hi Casey:
Will you continue to work on this solution or send a new version patches?
Thanks
Ding
> Casey
>
> .
>
^ permalink raw reply
* Re: [PATCH net] tcp: avoid fragmenting peculiar skbs in SACK
From: Neal Cardwell @ 2017-05-11 1:33 UTC (permalink / raw)
To: Yuchung Cheng; +Cc: David Miller, Netdev, Eric Dumazet, Soheil Hassas Yeganeh
In-Reply-To: <20170511000127.4249-1-ycheng@google.com>
On Wed, May 10, 2017 at 8:01 PM, Yuchung Cheng <ycheng@google.com> wrote:
>
> This patch fixes a bug in splitting an SKB during SACK
> processing. Specifically if an skb contains multiple
> packets and is only partially sacked in the higher sequences,
> tcp_match_sack_to_skb() splits the skb and marks the second fragment
> as SACKed.
>
> The current code further attempts rounding up the first fragment
> to MSS boundaries. But it misses a boundary condition when the
> rounded-up fragment size (pkt_len) is exactly skb size. Spliting
> such an skb is pointless and causses a kernel warning and aborts
> the SACK processing. This patch universally checks such over-split
> before calling tcp_fragment to prevent these unnecessary warnings.
>
> Fixes: adb92db857ee ("tcp: Make SACK code to split only at mss boundaries")
> Signed-off-by: Yuchung Cheng <ycheng@google.com>
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Signed-off-by: Soheil Hassas Yeganeh <soheil@google.com>
Acked-by: Neal Cardwell <ncardwell@google.com>
neal
^ permalink raw reply
* Re: openvswitch MTU patch needed in 4.10 stable
From: David Miller @ 2017-05-11 2:40 UTC (permalink / raw)
To: stephen; +Cc: netdev
In-Reply-To: <20170509084656.53f2d590@xeon-e3>
From: Stephen Hemminger <stephen@networkplumber.org>
Date: Tue, 9 May 2017 08:46:56 -0700
> Could you queue the patch to stable?
It speeds things along if you actually specify the SHA1 ID of the
specific commit being requested for a -stable backport.
I did this work, but you could have taken a brief amount of time so
that this would not have been necessary.
Thanks.
^ permalink raw reply
* Re: DQL and TCQ_F_CAN_BYPASS destroy performance under virtualizaiton (Was: "Re: net_sched strange in 4.11")
From: Jason Wang @ 2017-05-11 2:43 UTC (permalink / raw)
To: Anton Ivanov, Stefan Hajnoczi; +Cc: David S. Miller, netdev, Michael S. Tsirkin
In-Reply-To: <c9fe2748-0a14-17f4-3742-3184f0b2831b@cambridgegreys.com>
On 2017年05月10日 17:42, Anton Ivanov wrote:
> On 10/05/17 09:56, Jason Wang wrote:
>>
>>
>> On 2017年05月10日 13:28, Anton Ivanov wrote:
>>> On 10/05/17 03:18, Jason Wang wrote:
>>>>
>>>> On 2017年05月09日 23:11, Stefan Hajnoczi wrote:
>>>>> On Tue, May 09, 2017 at 08:46:46AM +0100, Anton Ivanov wrote:
>>>>>> I have figured it out. Two issues.
>>>>>>
>>>>>> 1) skb->xmit_more is hardly ever set under virtualization because
>>>>>> the qdisc
>>>>>> is usually bypassed because of TCQ_F_CAN_BYPASS. Once
>>>>>> TCQ_F_CAN_BYPASS is
>>>>>> set a virtual NIC driver is not likely see skb->xmit_more (this
>>>>>> answers my
>>>>>> "how does this work at all" question).
>>>>>>
>>>>>> 2) If that flag is turned off (I patched sched_generic to turn it
>>>>>> off in
>>>>>> pfifo_fast while testing), DQL keeps xmit_more from being set. If
>>>>>> the driver
>>>>>> is not DQL enabled xmit_more is never ever set. If the driver is DQL
>>>>>> enabled
>>>>>> the queue is adjusted to ensure xmit_more stops happening within
>>>>>> 10-15 xmit
>>>>>> cycles.
>>>>>>
>>>>>> That is plain *wrong* for virtual NICs - virtio, emulated NICs, etc.
>>>>>> There,
>>>>>> the BIG cost is telling the hypervisor that it needs to "kick" the
>>>>>> packets.
>>>>>> The cost of putting them into the vNIC buffers is negligible. You
>>>>>> want
>>>>>> xmit_more to happen - it makes between 50% and 300% (depending on
>>>>>> vNIC
>>>>>> design) difference. If there is no xmit_more the vNIC will
>>>>>> immediately
>>>>>> "kick" the hypervisor and try to signal that the packet needs to
>>>>>> move
>>>>>> straight away (as for example in virtio_net).
>>>> How do you measure the performance? TCP or just measure pps?
>>> In this particular case - tcp from guest. I have a couple of other
>>> benchmarks (forwarding, etc).
>>
>> One more question, is the number for virtio-net or other emulated vNIC?
>
> Other for now - you are cc-ed to keep you in the loop.
>
> Virtio is next on my list - I am revisiting the l2tpv3.c driver in
> QEMU and looking at how to preserve bulking by adding back sendmmsg
> (as well as a list of other features/transports).
>
> We had sendmmsg removed for the final inclusion in QEMU 2.1, it
> presently uses only recvmmsg so for the time being it does not care.
> That will most likely change once it starts using sendmmsg as well.
An issue is that qemu net API does not support bulking, do you plan to
add it?
Thanks
^ permalink raw reply
* Re: [PATCH net-next V4 10/10] vhost_net: try batch dequing from skb array
From: Jason Wang @ 2017-05-11 2:47 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: netdev, linux-kernel
In-Reply-To: <20170510152848-mutt-send-email-mst@kernel.org>
On 2017年05月10日 20:34, Michael S. Tsirkin wrote:
> On Wed, May 10, 2017 at 11:36:22AM +0800, Jason Wang wrote:
>> We used to dequeue one skb during recvmsg() from skb_array, this could
>> be inefficient because of the bad cache utilization and spinlock
>> touching for each packet. This patch tries to batch them by calling
>> batch dequeuing helpers explicitly on the exported skb array and pass
>> the skb back through msg_control for underlayer socket to finish the
>> userspace copying.
>>
>> Batch dequeuing is also the requirement for more batching improvement
>> on rx.
>>
>> Tests were done by pktgen on tap with XDP1 in guest on top of batch
>> zeroing:
>>
>> rx batch | pps
>>
>> 256 2.41Mpps (+6.16%)
>> 128 2.48Mpps (+8.80%)
>> 64 2.38Mpps (+3.96%) <- Default
>> 16 2.31Mpps (+1.76%)
>> 4 2.31Mpps (+1.76%)
>> 1 2.30Mpps (+1.32%)
>> 0 2.27Mpps (+7.48%)
>>
>> Signed-off-by: Jason Wang<jasowang@redhat.com>
>> ---
>> drivers/vhost/net.c | 117 +++++++++++++++++++++++++++++++++++++++++++++++++---
>> 1 file changed, 111 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
>> index 9b51989..fbaecf3 100644
>> --- a/drivers/vhost/net.c
>> +++ b/drivers/vhost/net.c
>> @@ -28,6 +28,8 @@
>> #include <linux/if_macvlan.h>
>> #include <linux/if_tap.h>
>> #include <linux/if_vlan.h>
>> +#include <linux/skb_array.h>
>> +#include <linux/skbuff.h>
>>
>> #include <net/sock.h>
>>
>> @@ -85,6 +87,13 @@ struct vhost_net_ubuf_ref {
>> struct vhost_virtqueue *vq;
>> };
>>
>> +#define VHOST_RX_BATCH 64
>> +struct vhost_net_buf {
>> + struct sk_buff *queue[VHOST_RX_BATCH];
>> + int tail;
>> + int head;
>> +};
>> +
> Do you strictly need to put this inline? This structure is quite big
> already. Do you see a measureabe difference if you make it
>
> struct sk_buff **queue;
> int tail;
> int head;
>
> ?
I don't.
>
> Will also make it easier to play with the size in the future
> should someone want to see how does it work e.g. for different
> ring sizes.
>
Ok, will do this in next version
Thanks
^ permalink raw reply
* Re: [PATCH] libertas: Avoid reading past end of buffer
From: Kalle Valo @ 2017-05-11 3:45 UTC (permalink / raw)
To: Joe Perches
Cc: Kees Cook, netdev, libertas-dev, linux-wireless, Daniel Micay,
linux-kernel
In-Reply-To: <1494457506.2028.1.camel@perches.com>
Joe Perches <joe@perches.com> writes:
> unrelated trivia:
>
> lbs_deb_enter is used incorrectly here at
> function exit as both enter and leave calls.
>
> That type of copy/paste defect may be common.
>
> $ git grep -w lbs_deb_enter | wc -l
> 148
> $ git grep -w lbs_deb_leave | wc -l
> 71
>
> One would expect these numbers to be the same.
>
> Another option would be to delete all these
> calls as ftrace function tracing works well.
Yeah, deleting all the enter/exit calls would be better.
--
Kalle Valo
^ permalink raw reply
* Re: [PATCH] net: fec: select queue depending on VLAN priority
From: Stefan Agner @ 2017-05-11 4:07 UTC (permalink / raw)
To: Andy Duan; +Cc: David Miller, andrew, festevam, netdev, linux-kernel
In-Reply-To: <AM4PR0401MB2260BA1BC2F65B36284CF7D2FFEC0@AM4PR0401MB2260.eurprd04.prod.outlook.com>
On 2017-05-09 19:42, Andy Duan wrote:
> From: David Miller <davem@davemloft.net> Sent: Tuesday, May 09, 2017 9:39 PM
>>To: stefan@agner.ch
>>Cc: Andy Duan <fugang.duan@nxp.com>; andrew@lunn.ch;
>>festevam@gmail.com; netdev@vger.kernel.org; linux-
>>kernel@vger.kernel.org
>>Subject: Re: [PATCH] net: fec: select queue depending on VLAN priority
>>
>>From: Stefan Agner <stefan@agner.ch>
>>Date: Mon, 8 May 2017 22:37:08 -0700
>>
>>> Since the addition of the multi queue code with commit 59d0f7465644
>>> ("net: fec: init multi queue date structure") the queue selection has
>>> been handelt by the default transmit queue selection implementation
>>> which tries to evenly distribute the traffic across all available
>>> queues. This selection presumes that the queues are using an equal
>>> priority, however, the queues 1 and 2 are actually of higher priority
>>> (the classification of the queues is enabled in fec_enet_enable_ring).
>>>
>>> This can lead to net scheduler warnings and continuous TX ring dumps
>>> when exercising the system with iperf.
>>>
>>> Use only queue 0 for all common traffic (no VLAN and P802.1p priority
>>> 0 and 1) and route level 2-7 through queue 1 and 2.
>>>
>>> Signed-off-by: Fugang Duan <fugang.duan@nxp.com>
>>> Fixes: 59d0f7465644 ("net: fec: init multi queue date structure")
>>
>>If the queues are used for prioritization, and it does not have multiple normal
>>priority level queues, multiqueue is not what the driver should have
>>implemented.
> Firstly, HW multiple queues support:
> - Traffic-shaping bandwidth distribution supports credit-based and
> round-robin-based policies. Either policy can be combined with
> time-based shaping.
> - AVB (Audio Video Bridging, IEEE 802.1Qav) features:
> * Credit-based bandwidth distribution policy can be combined with
> time-based shaping
> * AVB endpoint talker and listener support
> * Support for arbitration between different priority traffic (for
> example, AVB class A, AVB class B, and non-AVB)
> Round-robin-based policies:
> It has the same priority for three queues: In the round-robin QoS
> scheme, each queue is given an equal opportunity to transmit one
> frame. For example, if queue n has a frame to transmit, the queue
> transmits its frame. After queue n has transmitted its frame, or if
> queue n does not have a frame to transmit, queue n+1 is then allowed
> to transmit its frame, and so on.
>
> Credit-based policies:
> The AVB credit based shaper acts independently, per class, to control
> the bandwidth distribution between normal traffic and time-sensitive
> traffic with respect to the total link bandwidth available.
> Credit based shaper conbined with time-based shaping:
> - priority: ClassA queue > ClassB queue > best effort
> - ensure the queue bandwidth as user set based on time-based shaping
> algorithms (transmitter transmit frame from three queue in turn based
> on time-based shaping algorithms)
> And in real AVB case, each streaming can be independent, and are
> fixed on related queue. Then driver level should implement
> .ndo_select_queue() to put the streaming into related queue. That is
> what the patch did.
>
> The current driver config the three queue to credit-based policies
> (AVB), the patch seems no problem for the implementation. Do you have
> any suggestion ?
>
I tried using the round robin mode by adding this:
+ /* Set Round-Robin policy */
+ writel(1, fep->hwp + FEC_QOS_SCHEME);
After a while I got the warning non the less:
WARNING: CPU: 0 PID: 0 at net/sched/sch_generic.c:316
dev_watchdog+0x248/0x24c
NETDEV WATCHDOG: eth0 (fec): transmit queue 2 timed out
Modules linked in:
CPU: 0 PID: 0 Comm: swapper/0 Not tainted
4.11.0-rc1-00058-g56d22eced8bc-dirty #377
Hardware name: Freescale i.MX7 Dual (Device Tree)
[<c02266b0>] (unwind_backtrace) from [<c0222d7c>] (show_stack+0x10/0x14)
[<c0222d7c>] (show_stack) from [<c04d4098>] (dump_stack+0x78/0x8c)
[<c04d4098>] (dump_stack) from [<c0236548>] (__warn+0xe8/0x100)
[<c0236548>] (__warn) from [<c0236598>] (warn_slowpath_fmt+0x38/0x48)
[<c0236598>] (warn_slowpath_fmt) from [<c0805904>]
(dev_watchdog+0x248/0x24c)
[<c0805904>] (dev_watchdog) from [<c028a0e8>] (call_timer_fn+0x28/0x98)
[<c028a0e8>] (call_timer_fn) from [<c028a1f8>] (expire_timers+0xa0/0xac)
[<c028a1f8>] (expire_timers) from [<c028a2a0>]
(run_timer_softirq+0x9c/0x194)
[<c028a2a0>] (run_timer_softirq) from [<c023aaf8>]
(__do_softirq+0x114/0x234)
[<c023aaf8>] (__do_softirq) from [<c023aee4>] (irq_exit+0xcc/0x108)
[<c023aee4>] (irq_exit) from [<c0279920>]
(__handle_domain_irq+0x80/0xec)
[<c0279920>] (__handle_domain_irq) from [<c0201544>]
(gic_handle_irq+0x48/0x8c)
[<c0201544>] (gic_handle_irq) from [<c0223838>] (__irq_svc+0x58/0x8c)
Exception stack(0xc1001f28 to 0xc1001f70)
1f20: 00000001 00000000 00000000 c022fdc0 c1000000
c1003d80
1f40: c1003d34 c0e72ed0 c0bd9b04 c1001f80 00000000 00000000 00000000
c1001f78
1f60: c022048c c0220490 60000013 ffffffff
[<c0223838>] (__irq_svc) from [<c0220490>] (arch_cpu_idle+0x38/0x3c)
[<c0220490>] (arch_cpu_idle) from [<c026ec60>] (do_idle+0x170/0x204)
[<c026ec60>] (do_idle) from [<c026efac>] (cpu_startup_entry+0x18/0x1c)
[<c026efac>] (cpu_startup_entry) from [<c0e00c88>]
(start_kernel+0x394/0x3a0)
---[ end trace a474f341d40e0705 ]---
fec 30be0000.ethernet eth0: TX ring dump
I disabled the regular ring dump and only printed one line. It seems to
come up every 2 seconds, and checking cat /proc/interrupts showed that
queue 2 stayed at its last value (3562218):
58: 3091320 GIC-0 150 Level 30be0000.ethernet
59: 3562218 GIC-0 151 Level 30be0000.ethernet
60: 13377922 GIC-0 152 Level 30be0000.ethernet
--
Stefan
^ permalink raw reply
* RE: [PATCH] net: fec: select queue depending on VLAN priority
From: Andy Duan @ 2017-05-11 4:49 UTC (permalink / raw)
To: Stefan Agner
Cc: David Miller, andrew@lunn.ch, festevam@gmail.com,
netdev@vger.kernel.org, linux-kernel@vger.kernel.org
In-Reply-To: <ed62e3b2eb91f7ef795279e92e249c51@agner.ch>
From: Stefan Agner <stefan@agner.ch> Sent: Thursday, May 11, 2017 12:08 PM
>To: Andy Duan <fugang.duan@nxp.com>
>Cc: David Miller <davem@davemloft.net>; andrew@lunn.ch;
>festevam@gmail.com; netdev@vger.kernel.org; linux-
>kernel@vger.kernel.org
>Subject: Re: [PATCH] net: fec: select queue depending on VLAN priority
>
>On 2017-05-09 19:42, Andy Duan wrote:
>> From: David Miller <davem@davemloft.net> Sent: Tuesday, May 09, 2017
>> 9:39 PM
>>>To: stefan@agner.ch
>>>Cc: Andy Duan <fugang.duan@nxp.com>; andrew@lunn.ch;
>>>festevam@gmail.com; netdev@vger.kernel.org; linux-
>>>kernel@vger.kernel.org
>>>Subject: Re: [PATCH] net: fec: select queue depending on VLAN priority
>>>
>>>From: Stefan Agner <stefan@agner.ch>
>>>Date: Mon, 8 May 2017 22:37:08 -0700
>>>
>>>> Since the addition of the multi queue code with commit 59d0f7465644
>>>> ("net: fec: init multi queue date structure") the queue selection
>>>> has been handelt by the default transmit queue selection
>>>> implementation which tries to evenly distribute the traffic across
>>>> all available queues. This selection presumes that the queues are
>>>> using an equal priority, however, the queues 1 and 2 are actually of
>>>> higher priority (the classification of the queues is enabled in
>fec_enet_enable_ring).
>>>>
>>>> This can lead to net scheduler warnings and continuous TX ring dumps
>>>> when exercising the system with iperf.
>>>>
>>>> Use only queue 0 for all common traffic (no VLAN and P802.1p
>>>> priority
>>>> 0 and 1) and route level 2-7 through queue 1 and 2.
>>>>
>>>> Signed-off-by: Fugang Duan <fugang.duan@nxp.com>
>>>> Fixes: 59d0f7465644 ("net: fec: init multi queue date structure")
>>>
>>>If the queues are used for prioritization, and it does not have
>>>multiple normal priority level queues, multiqueue is not what the
>>>driver should have implemented.
>> Firstly, HW multiple queues support:
>> - Traffic-shaping bandwidth distribution supports credit-based and
>> round-robin-based policies. Either policy can be combined with
>> time-based shaping.
>> - AVB (Audio Video Bridging, IEEE 802.1Qav) features:
>> * Credit-based bandwidth distribution policy can be combined
>with
>> time-based shaping
>> * AVB endpoint talker and listener support
>> * Support for arbitration between different priority traffic (for
>> example, AVB class A, AVB class B, and non-AVB) Round-robin-based
>> policies:
>> It has the same priority for three queues: In the round-robin QoS
>> scheme, each queue is given an equal opportunity to transmit one
>> frame. For example, if queue n has a frame to transmit, the queue
>> transmits its frame. After queue n has transmitted its frame, or if
>> queue n does not have a frame to transmit, queue n+1 is then allowed
>> to transmit its frame, and so on.
>>
>> Credit-based policies:
>> The AVB credit based shaper acts independently, per class, to control
>> the bandwidth distribution between normal traffic and time-sensitive
>> traffic with respect to the total link bandwidth available.
>> Credit based shaper conbined with time-based shaping:
>> - priority: ClassA queue > ClassB queue > best effort
>> - ensure the queue bandwidth as user set based on time-
>based shaping
>> algorithms (transmitter transmit frame from three queue in turn based
>> on time-based shaping algorithms)
>> And in real AVB case, each streaming can be independent, and are
>> fixed on related queue. Then driver level should implement
>> .ndo_select_queue() to put the streaming into related queue. That is
>> what the patch did.
>>
>> The current driver config the three queue to credit-based policies
>> (AVB), the patch seems no problem for the implementation. Do you have
>> any suggestion ?
>>
>
>I tried using the round robin mode by adding this:
>
>+ /* Set Round-Robin policy */
>+ writel(1, fep->hwp + FEC_QOS_SCHEME);
>
>After a while I got the warning non the less:
>
>WARNING: CPU: 0 PID: 0 at net/sched/sch_generic.c:316
>dev_watchdog+0x248/0x24c NETDEV WATCHDOG: eth0 (fec): transmit queue
>2 timed out Modules linked in:
>CPU: 0 PID: 0 Comm: swapper/0 Not tainted 4.11.0-rc1-00058-g56d22eced8bc-
>dirty #377 Hardware name: Freescale i.MX7 Dual (Device Tree) [<c02266b0>]
>(unwind_backtrace) from [<c0222d7c>] (show_stack+0x10/0x14) [<c0222d7c>]
>(show_stack) from [<c04d4098>] (dump_stack+0x78/0x8c) [<c04d4098>]
>(dump_stack) from [<c0236548>] (__warn+0xe8/0x100) [<c0236548>]
>(__warn) from [<c0236598>] (warn_slowpath_fmt+0x38/0x48) [<c0236598>]
>(warn_slowpath_fmt) from [<c0805904>]
>(dev_watchdog+0x248/0x24c)
>[<c0805904>] (dev_watchdog) from [<c028a0e8>] (call_timer_fn+0x28/0x98)
>[<c028a0e8>] (call_timer_fn) from [<c028a1f8>] (expire_timers+0xa0/0xac)
>[<c028a1f8>] (expire_timers) from [<c028a2a0>]
>(run_timer_softirq+0x9c/0x194)
>[<c028a2a0>] (run_timer_softirq) from [<c023aaf8>]
>(__do_softirq+0x114/0x234)
>[<c023aaf8>] (__do_softirq) from [<c023aee4>] (irq_exit+0xcc/0x108)
>[<c023aee4>] (irq_exit) from [<c0279920>]
>(__handle_domain_irq+0x80/0xec)
>[<c0279920>] (__handle_domain_irq) from [<c0201544>]
>(gic_handle_irq+0x48/0x8c)
>[<c0201544>] (gic_handle_irq) from [<c0223838>] (__irq_svc+0x58/0x8c)
>Exception stack(0xc1001f28 to 0xc1001f70)
>1f20: 00000001 00000000 00000000 c022fdc0 c1000000
>c1003d80
>1f40: c1003d34 c0e72ed0 c0bd9b04 c1001f80 00000000 00000000 00000000
>c1001f78
>1f60: c022048c c0220490 60000013 ffffffff [<c0223838>] (__irq_svc) from
>[<c0220490>] (arch_cpu_idle+0x38/0x3c) [<c0220490>] (arch_cpu_idle) from
>[<c026ec60>] (do_idle+0x170/0x204) [<c026ec60>] (do_idle) from
>[<c026efac>] (cpu_startup_entry+0x18/0x1c) [<c026efac>]
>(cpu_startup_entry) from [<c0e00c88>]
>(start_kernel+0x394/0x3a0)
>---[ end trace a474f341d40e0705 ]---
>fec 30be0000.ethernet eth0: TX ring dump
>
>I disabled the regular ring dump and only printed one line. It seems to come
>up every 2 seconds, and checking cat /proc/interrupts showed that queue 2
>stayed at its last value (3562218):
>
> 58: 3091320 GIC-0 150 Level 30be0000.ethernet
> 59: 3562218 GIC-0 151 Level 30be0000.ethernet
> 60: 13377922 GIC-0 152 Level 30be0000.ethernet
Pls check ENETx_DMAnCFG[16] whether is set, and disable time-based shaping for round robin.
^ permalink raw reply
* [PATCH net] net: sched: optimize class dumps
From: Eric Dumazet @ 2017-05-11 4:59 UTC (permalink / raw)
To: David Miller; +Cc: netdev, Jiri Kosina, Jamal Hadi Salim, Cong Wang, Jiri Pirko
From: Eric Dumazet <edumazet@google.com>
In commit 59cc1f61f09c ("net: sched: convert qdisc linked list to
hashtable") we missed the opportunity to considerably speed up
tc_dump_tclass_root() if a qdisc handle is provided by user.
Instead of iterating all the qdiscs, use qdisc_match_from_root()
to directly get the one we look for.
Signed-off-by: Eric Dumazet <edumazet@google.com>
Cc: Jiri Kosina <jkosina@suse.cz>
Cc: Jamal Hadi Salim <jhs@mojatatu.com>
Cc: Cong Wang <xiyou.wangcong@gmail.com>
Cc: Jiri Pirko <jiri@resnulli.us>
---
net/sched/sch_api.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/net/sched/sch_api.c b/net/sched/sch_api.c
index bbe57d57b67fd498692bd41db49147511f1bb091..e88342fde1bc409aed6a3c86e7a628030eaac66f 100644
--- a/net/sched/sch_api.c
+++ b/net/sched/sch_api.c
@@ -1831,6 +1831,12 @@ static int tc_dump_tclass_root(struct Qdisc *root, struct sk_buff *skb,
if (!qdisc_dev(root))
return 0;
+ if (tcm->tcm_parent) {
+ q = qdisc_match_from_root(root, TC_H_MAJ(tcm->tcm_parent));
+ if (q && tc_dump_tclass_qdisc(q, skb, tcm, cb, t_p, s_t) < 0)
+ return -1;
+ return 0;
+ }
hash_for_each(qdisc_dev(root)->qdisc_hash, b, q, hash) {
if (tc_dump_tclass_qdisc(q, skb, tcm, cb, t_p, s_t) < 0)
return -1;
^ permalink raw reply related
* Re: DQL and TCQ_F_CAN_BYPASS destroy performance under virtualizaiton (Was: "Re: net_sched strange in 4.11")
From: Anton Ivanov @ 2017-05-11 5:43 UTC (permalink / raw)
To: Jason Wang, Stefan Hajnoczi; +Cc: David S. Miller, netdev, Michael S. Tsirkin
In-Reply-To: <c11c50db-4e66-f3c2-5ebb-519ad6dbc2fe@redhat.com>
On 11/05/17 03:43, Jason Wang wrote:
>
>
> On 2017年05月10日 17:42, Anton Ivanov wrote:
>> On 10/05/17 09:56, Jason Wang wrote:
>>>
>>>
>>> On 2017年05月10日 13:28, Anton Ivanov wrote:
>>>> On 10/05/17 03:18, Jason Wang wrote:
>>>>>
>>>>> On 2017年05月09日 23:11, Stefan Hajnoczi wrote:
>>>>>> On Tue, May 09, 2017 at 08:46:46AM +0100, Anton Ivanov wrote:
>>>>>>> I have figured it out. Two issues.
>>>>>>>
>>>>>>> 1) skb->xmit_more is hardly ever set under virtualization because
>>>>>>> the qdisc
>>>>>>> is usually bypassed because of TCQ_F_CAN_BYPASS. Once
>>>>>>> TCQ_F_CAN_BYPASS is
>>>>>>> set a virtual NIC driver is not likely see skb->xmit_more (this
>>>>>>> answers my
>>>>>>> "how does this work at all" question).
>>>>>>>
>>>>>>> 2) If that flag is turned off (I patched sched_generic to turn it
>>>>>>> off in
>>>>>>> pfifo_fast while testing), DQL keeps xmit_more from being set. If
>>>>>>> the driver
>>>>>>> is not DQL enabled xmit_more is never ever set. If the driver is
>>>>>>> DQL
>>>>>>> enabled
>>>>>>> the queue is adjusted to ensure xmit_more stops happening within
>>>>>>> 10-15 xmit
>>>>>>> cycles.
>>>>>>>
>>>>>>> That is plain *wrong* for virtual NICs - virtio, emulated NICs,
>>>>>>> etc.
>>>>>>> There,
>>>>>>> the BIG cost is telling the hypervisor that it needs to "kick" the
>>>>>>> packets.
>>>>>>> The cost of putting them into the vNIC buffers is negligible.
>>>>>>> You want
>>>>>>> xmit_more to happen - it makes between 50% and 300% (depending
>>>>>>> on vNIC
>>>>>>> design) difference. If there is no xmit_more the vNIC will
>>>>>>> immediately
>>>>>>> "kick" the hypervisor and try to signal that the packet needs
>>>>>>> to move
>>>>>>> straight away (as for example in virtio_net).
>>>>> How do you measure the performance? TCP or just measure pps?
>>>> In this particular case - tcp from guest. I have a couple of other
>>>> benchmarks (forwarding, etc).
>>>
>>> One more question, is the number for virtio-net or other emulated vNIC?
>>
>> Other for now - you are cc-ed to keep you in the loop.
>>
>> Virtio is next on my list - I am revisiting the l2tpv3.c driver in
>> QEMU and looking at how to preserve bulking by adding back sendmmsg
>> (as well as a list of other features/transports).
>>
>> We had sendmmsg removed for the final inclusion in QEMU 2.1, it
>> presently uses only recvmmsg so for the time being it does not care.
>> That will most likely change once it starts using sendmmsg as well.
>
> An issue is that qemu net API does not support bulking, do you plan to
> add it?
Yes :)
A.
>
> Thanks
>
--
Anton R. Ivanov
Cambridgegreys Limited. Registered in England. Company Number 10273661
^ permalink raw reply
* [PATCH] net/mlx4_core: Use min_t instead of if for consistency
From: Yuval Shaia @ 2017-05-11 8:20 UTC (permalink / raw)
To: yishaih-VPRAkNaXOzVWk0Htik3J/w, netdev-u79uwXL29TY76Z2rM5mHXA,
linux-rdma-u79uwXL29TY76Z2rM5mHXA
Signed-off-by: Yuval Shaia <yuval.shaia-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>
---
drivers/net/ethernet/mellanox/mlx4/main.c | 9 ++++-----
1 file changed, 4 insertions(+), 5 deletions(-)
diff --git a/drivers/net/ethernet/mellanox/mlx4/main.c b/drivers/net/ethernet/mellanox/mlx4/main.c
index 7032054..a58b15a 100644
--- a/drivers/net/ethernet/mellanox/mlx4/main.c
+++ b/drivers/net/ethernet/mellanox/mlx4/main.c
@@ -2862,12 +2862,11 @@ static void mlx4_enable_msi_x(struct mlx4_dev *dev)
int port = 0;
if (msi_x) {
- int nreq = dev->caps.num_ports * num_online_cpus() + 1;
+ int nreq = min_t(int,
+ dev->caps.num_ports * num_online_cpus() + 1,
+ dev->caps.num_eqs - dev->caps.reserved_eqs);
- nreq = min_t(int, dev->caps.num_eqs - dev->caps.reserved_eqs,
- nreq);
- if (nreq > MAX_MSIX)
- nreq = MAX_MSIX;
+ nreq = min_t(nreq, MAX_MSIX);
entries = kcalloc(nreq, sizeof *entries, GFP_KERNEL);
if (!entries)
--
2.7.4
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply related
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