* [PATCH] tcp: initialize max window for a new fastopen socket
From: Alexey Kodanev @ 2017-01-19 13:36 UTC (permalink / raw)
To: David Miller
Cc: netdev, Eric Dumazet, Yuchung Cheng, Neal Cardwell,
Alexey Kodanev
Found that if we run LTP netstress test with large MSS (65K),
the first attempt from server to send data comparable to this
MSS on fastopen connection will be delayed by the probe timer.
Here is an example:
< S seq 0:0 win 43690 options [mss 65495 wscale 7 tfo cookie] length 32
> S. seq 0:0 ack 1 win 43690 options [mss 65495 wscale 7] length 0
< . ack 1 win 342 length 0
Inside tcp_sendmsg(), tcp_send_mss() returns max MSS in 'mss_now',
as well as in 'size_goal'. This results the segment not queued for
transmition until all the data copied from user buffer. Then, inside
__tcp_push_pending_frames(), it breaks on send window test and
continues with the check probe timer.
Fragmentation occurs in tcp_write_wakeup()...
+0.2 > P. seq 1:43777 ack 1 win 342 length 43776
< . ack 43777, win 1365 length 0
> P. seq 43777:65001 ack 1 win 342 options [...] length 21224
...
This also contradicts with the fact that we should bound to the half
of the window if it is large.
Fix this flaw by correctly initializing max_window. Before that, it
could have large values that affect further calculations of 'size_goal'.
Fixes: 168a8f58059a ("tcp: TCP Fast Open Server - main code path")
Signed-off-by: Alexey Kodanev <alexey.kodanev@oracle.com>
---
net/ipv4/tcp_fastopen.c | 1 +
1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/net/ipv4/tcp_fastopen.c b/net/ipv4/tcp_fastopen.c
index f519195..dd2560c 100644
--- a/net/ipv4/tcp_fastopen.c
+++ b/net/ipv4/tcp_fastopen.c
@@ -205,6 +205,7 @@ void tcp_fastopen_add_skb(struct sock *sk, struct sk_buff *skb)
* scaled. So correct it appropriately.
*/
tp->snd_wnd = ntohs(tcp_hdr(skb)->window);
+ tp->max_window = tp->snd_wnd;
/* Activate the retrans timer so that SYNACK can be retransmitted.
* The request socket is not added to the ehash
--
1.7.1
^ permalink raw reply related
* Re: [net, 3/6] net: korina: increase tx/rx ring sizes
From: Eric Dumazet @ 2017-01-19 13:37 UTC (permalink / raw)
To: Roman Yeryomin; +Cc: David Miller, netdev
In-Reply-To: <CACiydbJV5djpKdsgnKTDfxHpC0aPhF-bdH=zVzGeo9x127RFJg@mail.gmail.com>
On Thu, 2017-01-19 at 12:55 +0200, Roman Yeryomin wrote:
> On 17 January 2017 at 21:20, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> > Note that at 100M, 64 rx descriptors have a 8 ms max latency.
> >
> > Switching to 256 also multiply by 4 the latency -> 32 ms latency.
> >
> > Presumably switching to NAPI and GRO would avoid the latency increase
> > and save a lot of cpu cycles for a MIPS board.
> >
>
> Eric, thanks for suggesting GRO, it gives huge performance gain when
> receiving locally (55->95Mbps) and more than 25% gain for NAT
> (55->70Mbps).
> Also reading the datasheet more carefully I see that device rx
> descriptor status flags are interpreted incorrectly. So will provide
> an updated set.
Oh right NAPI for rx is already implemented, so using GRO is a trivial
patch.
^ permalink raw reply
* Re: [PATCH] tcp: initialize max window for a new fastopen socket
From: Eric Dumazet @ 2017-01-19 13:41 UTC (permalink / raw)
To: Alexey Kodanev; +Cc: David Miller, netdev, Yuchung Cheng, Neal Cardwell
In-Reply-To: <1484832999-1849-1-git-send-email-alexey.kodanev@oracle.com>
On Thu, 2017-01-19 at 16:36 +0300, Alexey Kodanev wrote:
> Found that if we run LTP netstress test with large MSS (65K),
> the first attempt from server to send data comparable to this
> MSS on fastopen connection will be delayed by the probe timer.
>
> This also contradicts with the fact that we should bound to the half
> of the window if it is large.
>
> Fix this flaw by correctly initializing max_window. Before that, it
> could have large values that affect further calculations of 'size_goal'.
>
> Fixes: 168a8f58059a ("tcp: TCP Fast Open Server - main code path")
> Signed-off-by: Alexey Kodanev <alexey.kodanev@oracle.com>
Acked-by: Eric Dumazet <edumazet@google.com>
Thanks Alexey !
^ permalink raw reply
* Re: [PATCH] net: fec: Fixed panic problem with non-tso
From: Eric Dumazet @ 2017-01-19 13:24 UTC (permalink / raw)
To: Andy Duan; +Cc: Ashizuka, Yuusuke, netdev@vger.kernel.org, Pravin B Shelar
In-Reply-To: <AM4PR0401MB2260F19C276A1329FD2BB8A7FF7E0@AM4PR0401MB2260.eurprd04.prod.outlook.com>
On Thu, 2017-01-19 at 08:18 +0000, Andy Duan wrote:
> I will double check your fix. Thanks.
>
> And, if driver is to support highmem, then we should add tso highmem
> support in net/core/tso.c, do you think it is necessary ?
Adding TSO highmem support would mean changing net/core/tso.c ABI and
thus changing all net/core/tso.c users.
Looks a lot of work to me.
^ permalink raw reply
* Re: [PATCH iproute2 net-next V2] tc: flower: Refactor matching flags to be more user friendly
From: Paul Blakey @ 2017-01-19 14:14 UTC (permalink / raw)
To: Jiri Benc
Cc: paulb, Stephen Hemminger, netdev, Jiri Pirko, Or Gerlitz,
Roi Dayan, Simon Horman
In-Reply-To: <20170119150447.6b9ad397@griffin>
On 19/01/2017 16:04, Jiri Benc wrote:
> On Thu, 19 Jan 2017 14:17:57 +0200, Paul Blakey wrote:
>> Instead of "magic numbers" we can now specify each flag
>> by name. Prefix of "no_" (e.g no_frag) unsets the flag,
>> otherwise it wil be set.
>
> Other parts of the tc tool use the "no" prefix (without the
> underscore), such as "nofrag" in u32 and pedit, "noecn" in fq_codel and
> pie, etc. We should follow the suit here.
>
> Looks good to me otherwise, thanks a lot for doing the work!
>
> Jiri
>
Thanks, v3 is up.
^ permalink raw reply
* Re: [PATCH iproute2 net-next V3] tc: flower: Refactor matching flags to be more user friendly
From: Paul Blakey @ 2017-01-19 14:19 UTC (permalink / raw)
To: Stephen Hemminger, netdev
Cc: paulb, Jiri Pirko, Or Gerlitz, Roi Dayan, Jiri Benc, Simon Horman
In-Reply-To: <1484835241-45689-1-git-send-email-paulb@mellanox.com>
On 19/01/2017 16:14, Paul Blakey wrote:
> Instead of "magic numbers" we can now specify each flag
> by name. Prefix of "no" (e.g nofrag) unsets the flag,
> otherwise it wil be set.
>
> Example:
> # add a flower filter that will drop fragmented packets
> tc filter add dev ens4f0 protocol ip parent ffff: \
> flower \
> src_mac e4:1d:2d:fd:8b:01 \
> dst_mac e4:1d:2d:fd:8b:02 \
> indev ens4f0 \
> ip_flags frag \
> action drop
>
> # add a flower filter that will drop non-fragmented packets
> tc filter add dev ens4f0 protocol ip parent ffff: \
> flower \
> src_mac e4:1d:2d:fd:8b:01 \
> dst_mac e4:1d:2d:fd:8b:02 \
> indev ens4f0 \
> ip_flags nofrag \
> action drop
>
> Fixes: 22a8f019891c ('tc: flower: support matching flags')
> Signed-off-by: Paul Blakey <paulb@mellanox.com>
> Reviewed-by: Roi Dayan <roid@mellanox.com>
> ---
>
> Hi,
> Added a framework to add new flags more easily, such
> as the upcoming tcp_flags (see kernel cls_flower), and other ip_flags.
>
> Thanks,
> Paul.
>
>
> Changelog:
>
> v3:
> Changed prefix to "no" instead of "no_".
>
> v2:
> Changed delimiter to "/" to avoid shell pipe errors.
>
>
> man/man8/tc-flower.8 | 12 +++++-
> tc/f_flower.c | 117 ++++++++++++++++++++++++++++++++++++++++-----------
> 2 files changed, 102 insertions(+), 27 deletions(-)
>
> diff --git a/man/man8/tc-flower.8 b/man/man8/tc-flower.8
> index 2dd2c5e..77da10b 100644
> --- a/man/man8/tc-flower.8
> +++ b/man/man8/tc-flower.8
> @@ -46,7 +46,9 @@ flower \- flow based traffic control filter
> .BR enc_dst_ip " | " enc_src_ip " } { "
> .IR ipv4_address " | " ipv6_address " } | "
> .B enc_dst_port
> -.IR port_number
> +.IR port_number " | "
> +.BR ip_flags
> +.IR IP_FLAGS
> .SH DESCRIPTION
> The
> .B flower
> @@ -183,13 +185,19 @@ prefix length. If the prefix is missing, \fBtc\fR assumes a full-length
> host match. Dst port
> .I NUMBER
> is a 16 bit UDP dst port.
> +.TP
> +.BI ip_flags " IP_FLAGS"
> +.I IP_FLAGS
> +may be either
> +.BR frag " or " no_frag
> +to match on fragmented packets or not respectively.
> .SH NOTES
> As stated above where applicable, matches of a certain layer implicitly depend
> on the matches of the next lower layer. Precisely, layer one and two matches
> (\fBindev\fR, \fBdst_mac\fR and \fBsrc_mac\fR)
> have no dependency, layer three matches
> (\fBip_proto\fR, \fBdst_ip\fR, \fBsrc_ip\fR, \fBarp_tip\fR, \fBarp_sip\fR,
> -\fBarp_op\fR, \fBarp_tha\fR and \fBarp_sha\fR)
> +\fBarp_op\fR, \fBarp_tha\fR, \fBarp_sha\fR and \fBip_flags\fR)
> depend on the
> .B protocol
> option of tc filter, layer four port matches
> diff --git a/tc/f_flower.c b/tc/f_flower.c
> index d301db3..3dffe2b 100644
> --- a/tc/f_flower.c
> +++ b/tc/f_flower.c
> @@ -24,6 +24,10 @@
> #include "tc_util.h"
> #include "rt_names.h"
>
> +enum flower_matching_flags {
> + FLOWER_IP_FLAGS,
> +};
> +
> enum flower_endpoint {
> FLOWER_ENDPOINT_SRC,
> FLOWER_ENDPOINT_DST
> @@ -63,7 +67,7 @@ static void explain(void)
> " enc_dst_ip [ IPV4-ADDR | IPV6-ADDR ] |\n"
> " enc_src_ip [ IPV4-ADDR | IPV6-ADDR ] |\n"
> " enc_key_id [ KEY-ID ] |\n"
> - " matching_flags MATCHING-FLAGS | \n"
> + " ip_flags IP-FLAGS | \n"
> " enc_dst_port [ port_number ] }\n"
> " FILTERID := X:Y:Z\n"
> " MASKED_LLADDR := { LLADDR | LLADDR/MASK | LLADDR/BITS }\n"
> @@ -136,28 +140,56 @@ static int flower_parse_vlan_eth_type(char *str, __be16 eth_type, int type,
> return 0;
> }
>
> -static int flower_parse_matching_flags(char *str, int type, int mask_type,
> - struct nlmsghdr *n)
> -{
> - __u32 mtf, mtf_mask;
> - char *c;
> +struct flag_to_string {
> + int flag;
> + enum flower_matching_flags type;
> + char *string;
> +};
>
> - c = strchr(str, '/');
> - if (c)
> - *c = '\0';
> +static struct flag_to_string flags_str[] = {
> + { TCA_FLOWER_KEY_FLAGS_IS_FRAGMENT, FLOWER_IP_FLAGS, "frag" },
> +};
>
> - if (get_u32(&mtf, str, 0))
> - return -1;
> +static int flower_parse_matching_flags(char *str,
> + enum flower_matching_flags type,
> + __u32 *mtf, __u32 *mtf_mask)
> +{
> + char *token;
> + bool no;
> + bool found;
> + int i;
>
> - if (c) {
> - if (get_u32(&mtf_mask, ++c, 0))
> + token = strtok(str, "/");
> +
> + while (token) {
> + if (!strncmp(token, "no", 2)) {
> + no = true;
> + token = strchr(token, '_') + 1;
> + } else
> + no = false;
> +
> + found = false;
> + for (i = 0; i < ARRAY_SIZE(flags_str); i++) {
> + if (type != flags_str[i].type)
> + continue;
> +
> + if (!strcmp(token, flags_str[i].string)) {
> + if (no)
> + *mtf &= ~flags_str[i].flag;
> + else
> + *mtf |= flags_str[i].flag;
> +
> + *mtf_mask |= flags_str[i].flag;
> + found = true;
> + break;
> + }
> + }
> + if (!found)
> return -1;
> - } else {
> - mtf_mask = 0xffffffff;
> +
> + token = strtok(NULL, "/");
> }
>
> - addattr32(n, MAX_MSG, type, htonl(mtf));
> - addattr32(n, MAX_MSG, mask_type, htonl(mtf_mask));
> return 0;
> }
>
> @@ -433,6 +465,8 @@ static int flower_parse_opt(struct filter_util *qu, char *handle,
> __be16 vlan_ethtype = 0;
> __u8 ip_proto = 0xff;
> __u32 flags = 0;
> + __u32 mtf = 0;
> + __u32 mtf_mask = 0;
>
> if (handle) {
> ret = get_u32(&t->tcm_handle, handle, 0);
> @@ -462,14 +496,14 @@ static int flower_parse_opt(struct filter_util *qu, char *handle,
> return -1;
> }
> addattr_l(n, MAX_MSG, TCA_FLOWER_CLASSID, &handle, 4);
> - } else if (matches(*argv, "matching_flags") == 0) {
> + } else if (matches(*argv, "ip_flags") == 0) {
> NEXT_ARG();
> ret = flower_parse_matching_flags(*argv,
> - TCA_FLOWER_KEY_FLAGS,
> - TCA_FLOWER_KEY_FLAGS_MASK,
> - n);
> + FLOWER_IP_FLAGS,
> + &mtf,
> + &mtf_mask);
> if (ret < 0) {
> - fprintf(stderr, "Illegal \"matching_flags\"\n");
> + fprintf(stderr, "Illegal \"ip_flags\"\n");
> return -1;
> }
> } else if (matches(*argv, "skip_hw") == 0) {
> @@ -723,6 +757,16 @@ static int flower_parse_opt(struct filter_util *qu, char *handle,
> parse_done:
> addattr32(n, MAX_MSG, TCA_FLOWER_FLAGS, flags);
>
> + if (mtf_mask) {
> + ret = addattr32(n, MAX_MSG, TCA_FLOWER_KEY_FLAGS, htonl(mtf));
> + if (ret)
> + return ret;
> +
> + ret = addattr32(n, MAX_MSG, TCA_FLOWER_KEY_FLAGS_MASK, htonl(mtf_mask));
> + if (ret)
> + return ret;
> + }
> +
> ret = addattr16(n, MAX_MSG, TCA_FLOWER_KEY_ETH_TYPE, eth_type);
> if (ret) {
> fprintf(stderr, "Illegal \"eth_type\"(0x%x)\n",
> @@ -828,14 +872,36 @@ static void flower_print_ip_proto(FILE *f, __u8 *p_ip_proto,
> }
>
> static void flower_print_matching_flags(FILE *f, char *name,
> + enum flower_matching_flags type,
> struct rtattr *attr,
> struct rtattr *mask_attr)
> {
> + int i;
> + int count = 0;
> + __u32 mtf;
> + __u32 mtf_mask;
> +
> if (!mask_attr || RTA_PAYLOAD(mask_attr) != 4)
> return;
>
> - fprintf(f, "\n %s 0x%08x/0x%08x", name, ntohl(rta_getattr_u32(attr)),
> - mask_attr ? ntohl(rta_getattr_u32(mask_attr)) : 0xffffffff);
> + mtf = ntohl(rta_getattr_u32(attr));
> + mtf_mask = ntohl(rta_getattr_u32(mask_attr));
> +
> + for (i = 0; i < ARRAY_SIZE(flags_str); i++) {
> + if (type != flags_str[i].type)
> + continue;
> + if (mtf_mask & flags_str[i].flag) {
> + if (++count == 1)
> + fprintf(f, "\n %s ", name);
> + else
> + fprintf(f, "/");
> +
> + if (mtf & flags_str[i].flag)
> + fprintf(f, "%s", flags_str[i].string);
> + else
> + fprintf(f, "no%s", flags_str[i].string);
> + }
> + }
> }
>
> static void flower_print_ip_addr(FILE *f, char *name, __be16 eth_type,
> @@ -1034,7 +1100,8 @@ static int flower_print_opt(struct filter_util *qu, FILE *f,
> flower_print_port(f, "enc_dst_port",
> tb[TCA_FLOWER_KEY_ENC_UDP_DST_PORT]);
>
> - flower_print_matching_flags(f, "matching_flags",
> + flower_print_matching_flags(f, "ip_flags",
> + FLOWER_IP_FLAGS,
> tb[TCA_FLOWER_KEY_FLAGS],
> tb[TCA_FLOWER_KEY_FLAGS_MASK]);
>
>
Forgot to update man page, v4 is up.
^ permalink raw reply
* RE: [PATCH net] gianfar: Do not reuse pages from emergency reserve
From: Claudiu Manoil @ 2017-01-19 14:04 UTC (permalink / raw)
To: Eric Dumazet, David Miller; +Cc: netdev
In-Reply-To: <1484797482.16328.13.camel@edumazet-glaptop3.roam.corp.google.com>
>-----Original Message-----
>From: Eric Dumazet [mailto:eric.dumazet@gmail.com]
>Sent: Thursday, January 19, 2017 5:45 AM
>To: David Miller <davem@davemloft.net>
>Cc: netdev <netdev@vger.kernel.org>; Claudiu Manoil
><claudiu.manoil@freescale.com>
>Subject: [PATCH net] gianfar: Do not reuse pages from emergency reserve
>
>From: Eric Dumazet <edumazet@google.com>
>
>A driver using dev_alloc_page() must not reuse a page that had to
>use emergency memory reserve.
>
>Otherwise all packets using this page will be immediately dropped,
>unless for very specific sockets having SOCK_MEMALLOC bit set.
>
>This issue might be hard to debug, because only a fraction of the RX
>ring buffer would suffer from drops.
>
>Fixes: 75354148ce69 ("gianfar: Add paged allocation and Rx S/G")
>Signed-off-by: Eric Dumazet <edumazet@google.com>
>Cc: Claudiu Manoil <claudiu.manoil@freescale.com>
Thanks.
Acked-by: Claudiu Manoil <claudiu.manoil@nxp.com>
^ permalink raw reply
* [PATCH iproute2 net-next V3] tc: flower: Refactor matching flags to be more user friendly
From: Paul Blakey @ 2017-01-19 14:14 UTC (permalink / raw)
To: Stephen Hemminger, netdev
Cc: Jiri Pirko, Or Gerlitz, Roi Dayan, Jiri Benc, Simon Horman,
Paul Blakey
Instead of "magic numbers" we can now specify each flag
by name. Prefix of "no" (e.g nofrag) unsets the flag,
otherwise it wil be set.
Example:
# add a flower filter that will drop fragmented packets
tc filter add dev ens4f0 protocol ip parent ffff: \
flower \
src_mac e4:1d:2d:fd:8b:01 \
dst_mac e4:1d:2d:fd:8b:02 \
indev ens4f0 \
ip_flags frag \
action drop
# add a flower filter that will drop non-fragmented packets
tc filter add dev ens4f0 protocol ip parent ffff: \
flower \
src_mac e4:1d:2d:fd:8b:01 \
dst_mac e4:1d:2d:fd:8b:02 \
indev ens4f0 \
ip_flags nofrag \
action drop
Fixes: 22a8f019891c ('tc: flower: support matching flags')
Signed-off-by: Paul Blakey <paulb@mellanox.com>
Reviewed-by: Roi Dayan <roid@mellanox.com>
---
Hi,
Added a framework to add new flags more easily, such
as the upcoming tcp_flags (see kernel cls_flower), and other ip_flags.
Thanks,
Paul.
Changelog:
v3:
Changed prefix to "no" instead of "no_".
v2:
Changed delimiter to "/" to avoid shell pipe errors.
man/man8/tc-flower.8 | 12 +++++-
tc/f_flower.c | 117 ++++++++++++++++++++++++++++++++++++++++-----------
2 files changed, 102 insertions(+), 27 deletions(-)
diff --git a/man/man8/tc-flower.8 b/man/man8/tc-flower.8
index 2dd2c5e..77da10b 100644
--- a/man/man8/tc-flower.8
+++ b/man/man8/tc-flower.8
@@ -46,7 +46,9 @@ flower \- flow based traffic control filter
.BR enc_dst_ip " | " enc_src_ip " } { "
.IR ipv4_address " | " ipv6_address " } | "
.B enc_dst_port
-.IR port_number
+.IR port_number " | "
+.BR ip_flags
+.IR IP_FLAGS
.SH DESCRIPTION
The
.B flower
@@ -183,13 +185,19 @@ prefix length. If the prefix is missing, \fBtc\fR assumes a full-length
host match. Dst port
.I NUMBER
is a 16 bit UDP dst port.
+.TP
+.BI ip_flags " IP_FLAGS"
+.I IP_FLAGS
+may be either
+.BR frag " or " no_frag
+to match on fragmented packets or not respectively.
.SH NOTES
As stated above where applicable, matches of a certain layer implicitly depend
on the matches of the next lower layer. Precisely, layer one and two matches
(\fBindev\fR, \fBdst_mac\fR and \fBsrc_mac\fR)
have no dependency, layer three matches
(\fBip_proto\fR, \fBdst_ip\fR, \fBsrc_ip\fR, \fBarp_tip\fR, \fBarp_sip\fR,
-\fBarp_op\fR, \fBarp_tha\fR and \fBarp_sha\fR)
+\fBarp_op\fR, \fBarp_tha\fR, \fBarp_sha\fR and \fBip_flags\fR)
depend on the
.B protocol
option of tc filter, layer four port matches
diff --git a/tc/f_flower.c b/tc/f_flower.c
index d301db3..3dffe2b 100644
--- a/tc/f_flower.c
+++ b/tc/f_flower.c
@@ -24,6 +24,10 @@
#include "tc_util.h"
#include "rt_names.h"
+enum flower_matching_flags {
+ FLOWER_IP_FLAGS,
+};
+
enum flower_endpoint {
FLOWER_ENDPOINT_SRC,
FLOWER_ENDPOINT_DST
@@ -63,7 +67,7 @@ static void explain(void)
" enc_dst_ip [ IPV4-ADDR | IPV6-ADDR ] |\n"
" enc_src_ip [ IPV4-ADDR | IPV6-ADDR ] |\n"
" enc_key_id [ KEY-ID ] |\n"
- " matching_flags MATCHING-FLAGS | \n"
+ " ip_flags IP-FLAGS | \n"
" enc_dst_port [ port_number ] }\n"
" FILTERID := X:Y:Z\n"
" MASKED_LLADDR := { LLADDR | LLADDR/MASK | LLADDR/BITS }\n"
@@ -136,28 +140,56 @@ static int flower_parse_vlan_eth_type(char *str, __be16 eth_type, int type,
return 0;
}
-static int flower_parse_matching_flags(char *str, int type, int mask_type,
- struct nlmsghdr *n)
-{
- __u32 mtf, mtf_mask;
- char *c;
+struct flag_to_string {
+ int flag;
+ enum flower_matching_flags type;
+ char *string;
+};
- c = strchr(str, '/');
- if (c)
- *c = '\0';
+static struct flag_to_string flags_str[] = {
+ { TCA_FLOWER_KEY_FLAGS_IS_FRAGMENT, FLOWER_IP_FLAGS, "frag" },
+};
- if (get_u32(&mtf, str, 0))
- return -1;
+static int flower_parse_matching_flags(char *str,
+ enum flower_matching_flags type,
+ __u32 *mtf, __u32 *mtf_mask)
+{
+ char *token;
+ bool no;
+ bool found;
+ int i;
- if (c) {
- if (get_u32(&mtf_mask, ++c, 0))
+ token = strtok(str, "/");
+
+ while (token) {
+ if (!strncmp(token, "no", 2)) {
+ no = true;
+ token = strchr(token, '_') + 1;
+ } else
+ no = false;
+
+ found = false;
+ for (i = 0; i < ARRAY_SIZE(flags_str); i++) {
+ if (type != flags_str[i].type)
+ continue;
+
+ if (!strcmp(token, flags_str[i].string)) {
+ if (no)
+ *mtf &= ~flags_str[i].flag;
+ else
+ *mtf |= flags_str[i].flag;
+
+ *mtf_mask |= flags_str[i].flag;
+ found = true;
+ break;
+ }
+ }
+ if (!found)
return -1;
- } else {
- mtf_mask = 0xffffffff;
+
+ token = strtok(NULL, "/");
}
- addattr32(n, MAX_MSG, type, htonl(mtf));
- addattr32(n, MAX_MSG, mask_type, htonl(mtf_mask));
return 0;
}
@@ -433,6 +465,8 @@ static int flower_parse_opt(struct filter_util *qu, char *handle,
__be16 vlan_ethtype = 0;
__u8 ip_proto = 0xff;
__u32 flags = 0;
+ __u32 mtf = 0;
+ __u32 mtf_mask = 0;
if (handle) {
ret = get_u32(&t->tcm_handle, handle, 0);
@@ -462,14 +496,14 @@ static int flower_parse_opt(struct filter_util *qu, char *handle,
return -1;
}
addattr_l(n, MAX_MSG, TCA_FLOWER_CLASSID, &handle, 4);
- } else if (matches(*argv, "matching_flags") == 0) {
+ } else if (matches(*argv, "ip_flags") == 0) {
NEXT_ARG();
ret = flower_parse_matching_flags(*argv,
- TCA_FLOWER_KEY_FLAGS,
- TCA_FLOWER_KEY_FLAGS_MASK,
- n);
+ FLOWER_IP_FLAGS,
+ &mtf,
+ &mtf_mask);
if (ret < 0) {
- fprintf(stderr, "Illegal \"matching_flags\"\n");
+ fprintf(stderr, "Illegal \"ip_flags\"\n");
return -1;
}
} else if (matches(*argv, "skip_hw") == 0) {
@@ -723,6 +757,16 @@ static int flower_parse_opt(struct filter_util *qu, char *handle,
parse_done:
addattr32(n, MAX_MSG, TCA_FLOWER_FLAGS, flags);
+ if (mtf_mask) {
+ ret = addattr32(n, MAX_MSG, TCA_FLOWER_KEY_FLAGS, htonl(mtf));
+ if (ret)
+ return ret;
+
+ ret = addattr32(n, MAX_MSG, TCA_FLOWER_KEY_FLAGS_MASK, htonl(mtf_mask));
+ if (ret)
+ return ret;
+ }
+
ret = addattr16(n, MAX_MSG, TCA_FLOWER_KEY_ETH_TYPE, eth_type);
if (ret) {
fprintf(stderr, "Illegal \"eth_type\"(0x%x)\n",
@@ -828,14 +872,36 @@ static void flower_print_ip_proto(FILE *f, __u8 *p_ip_proto,
}
static void flower_print_matching_flags(FILE *f, char *name,
+ enum flower_matching_flags type,
struct rtattr *attr,
struct rtattr *mask_attr)
{
+ int i;
+ int count = 0;
+ __u32 mtf;
+ __u32 mtf_mask;
+
if (!mask_attr || RTA_PAYLOAD(mask_attr) != 4)
return;
- fprintf(f, "\n %s 0x%08x/0x%08x", name, ntohl(rta_getattr_u32(attr)),
- mask_attr ? ntohl(rta_getattr_u32(mask_attr)) : 0xffffffff);
+ mtf = ntohl(rta_getattr_u32(attr));
+ mtf_mask = ntohl(rta_getattr_u32(mask_attr));
+
+ for (i = 0; i < ARRAY_SIZE(flags_str); i++) {
+ if (type != flags_str[i].type)
+ continue;
+ if (mtf_mask & flags_str[i].flag) {
+ if (++count == 1)
+ fprintf(f, "\n %s ", name);
+ else
+ fprintf(f, "/");
+
+ if (mtf & flags_str[i].flag)
+ fprintf(f, "%s", flags_str[i].string);
+ else
+ fprintf(f, "no%s", flags_str[i].string);
+ }
+ }
}
static void flower_print_ip_addr(FILE *f, char *name, __be16 eth_type,
@@ -1034,7 +1100,8 @@ static int flower_print_opt(struct filter_util *qu, FILE *f,
flower_print_port(f, "enc_dst_port",
tb[TCA_FLOWER_KEY_ENC_UDP_DST_PORT]);
- flower_print_matching_flags(f, "matching_flags",
+ flower_print_matching_flags(f, "ip_flags",
+ FLOWER_IP_FLAGS,
tb[TCA_FLOWER_KEY_FLAGS],
tb[TCA_FLOWER_KEY_FLAGS_MASK]);
--
2.7.4
^ permalink raw reply related
* Re: [PATCH net-next V4] tc: flower: Refactor matching flags to be more user friendly
From: Jiri Benc @ 2017-01-19 14:22 UTC (permalink / raw)
To: Paul Blakey
Cc: Stephen Hemminger, netdev, Jiri Pirko, Or Gerlitz, Roi Dayan,
Simon Horman
In-Reply-To: <1484835468-46051-1-git-send-email-paulb@mellanox.com>
On Thu, 19 Jan 2017 16:17:48 +0200, Paul Blakey wrote:
> + while (token) {
> + if (!strncmp(token, "no", 2)) {
> + no = true;
> + token = strchr(token, '_') + 1;
This seems to still assume that "no" is followed by an underscore.
What about a simple token += 2?
Jiri
^ permalink raw reply
* [PATCH net-next V4] tc: flower: Refactor matching flags to be more user friendly
From: Paul Blakey @ 2017-01-19 14:17 UTC (permalink / raw)
To: Stephen Hemminger, netdev
Cc: Jiri Pirko, Or Gerlitz, Roi Dayan, Jiri Benc, Simon Horman,
Paul Blakey
Instead of "magic numbers" we can now specify each flag
by name. Prefix of "no" (e.g nofrag) unsets the flag,
otherwise it wil be set.
Example:
# add a flower filter that will drop fragmented packets
tc filter add dev ens4f0 protocol ip parent ffff: \
flower \
src_mac e4:1d:2d:fd:8b:01 \
dst_mac e4:1d:2d:fd:8b:02 \
indev ens4f0 \
ip_flags frag \
action drop
# add a flower filter that will drop non-fragmented packets
tc filter add dev ens4f0 protocol ip parent ffff: \
flower \
src_mac e4:1d:2d:fd:8b:01 \
dst_mac e4:1d:2d:fd:8b:02 \
indev ens4f0 \
ip_flags nofrag \
action drop
Fixes: 22a8f019891c ('tc: flower: support matching flags')
Signed-off-by: Paul Blakey <paulb@mellanox.com>
Reviewed-by: Roi Dayan <roid@mellanox.com>
---
Hi,
Added a framework to add new flags more easily, such
as the upcoming tcp_flags (see kernel cls_flower), and other ip_flags.
Thanks,
Paul.
Changelog:
v4:
Changed prefix in manpage as well.
v3:
Changed prefix to "no" instead of "no_".
v2:
Changed delimiter to "/" to avoid shell pipe errors.
man/man8/tc-flower.8 | 12 +++++-
tc/f_flower.c | 117 ++++++++++++++++++++++++++++++++++++++++-----------
2 files changed, 102 insertions(+), 27 deletions(-)
diff --git a/man/man8/tc-flower.8 b/man/man8/tc-flower.8
index 2dd2c5e..2bdd2ef 100644
--- a/man/man8/tc-flower.8
+++ b/man/man8/tc-flower.8
@@ -46,7 +46,9 @@ flower \- flow based traffic control filter
.BR enc_dst_ip " | " enc_src_ip " } { "
.IR ipv4_address " | " ipv6_address " } | "
.B enc_dst_port
-.IR port_number
+.IR port_number " | "
+.BR ip_flags
+.IR IP_FLAGS
.SH DESCRIPTION
The
.B flower
@@ -183,13 +185,19 @@ prefix length. If the prefix is missing, \fBtc\fR assumes a full-length
host match. Dst port
.I NUMBER
is a 16 bit UDP dst port.
+.TP
+.BI ip_flags " IP_FLAGS"
+.I IP_FLAGS
+may be either
+.BR frag " or " nofrag
+to match on fragmented packets or not respectively.
.SH NOTES
As stated above where applicable, matches of a certain layer implicitly depend
on the matches of the next lower layer. Precisely, layer one and two matches
(\fBindev\fR, \fBdst_mac\fR and \fBsrc_mac\fR)
have no dependency, layer three matches
(\fBip_proto\fR, \fBdst_ip\fR, \fBsrc_ip\fR, \fBarp_tip\fR, \fBarp_sip\fR,
-\fBarp_op\fR, \fBarp_tha\fR and \fBarp_sha\fR)
+\fBarp_op\fR, \fBarp_tha\fR, \fBarp_sha\fR and \fBip_flags\fR)
depend on the
.B protocol
option of tc filter, layer four port matches
diff --git a/tc/f_flower.c b/tc/f_flower.c
index d301db3..3dffe2b 100644
--- a/tc/f_flower.c
+++ b/tc/f_flower.c
@@ -24,6 +24,10 @@
#include "tc_util.h"
#include "rt_names.h"
+enum flower_matching_flags {
+ FLOWER_IP_FLAGS,
+};
+
enum flower_endpoint {
FLOWER_ENDPOINT_SRC,
FLOWER_ENDPOINT_DST
@@ -63,7 +67,7 @@ static void explain(void)
" enc_dst_ip [ IPV4-ADDR | IPV6-ADDR ] |\n"
" enc_src_ip [ IPV4-ADDR | IPV6-ADDR ] |\n"
" enc_key_id [ KEY-ID ] |\n"
- " matching_flags MATCHING-FLAGS | \n"
+ " ip_flags IP-FLAGS | \n"
" enc_dst_port [ port_number ] }\n"
" FILTERID := X:Y:Z\n"
" MASKED_LLADDR := { LLADDR | LLADDR/MASK | LLADDR/BITS }\n"
@@ -136,28 +140,56 @@ static int flower_parse_vlan_eth_type(char *str, __be16 eth_type, int type,
return 0;
}
-static int flower_parse_matching_flags(char *str, int type, int mask_type,
- struct nlmsghdr *n)
-{
- __u32 mtf, mtf_mask;
- char *c;
+struct flag_to_string {
+ int flag;
+ enum flower_matching_flags type;
+ char *string;
+};
- c = strchr(str, '/');
- if (c)
- *c = '\0';
+static struct flag_to_string flags_str[] = {
+ { TCA_FLOWER_KEY_FLAGS_IS_FRAGMENT, FLOWER_IP_FLAGS, "frag" },
+};
- if (get_u32(&mtf, str, 0))
- return -1;
+static int flower_parse_matching_flags(char *str,
+ enum flower_matching_flags type,
+ __u32 *mtf, __u32 *mtf_mask)
+{
+ char *token;
+ bool no;
+ bool found;
+ int i;
- if (c) {
- if (get_u32(&mtf_mask, ++c, 0))
+ token = strtok(str, "/");
+
+ while (token) {
+ if (!strncmp(token, "no", 2)) {
+ no = true;
+ token = strchr(token, '_') + 1;
+ } else
+ no = false;
+
+ found = false;
+ for (i = 0; i < ARRAY_SIZE(flags_str); i++) {
+ if (type != flags_str[i].type)
+ continue;
+
+ if (!strcmp(token, flags_str[i].string)) {
+ if (no)
+ *mtf &= ~flags_str[i].flag;
+ else
+ *mtf |= flags_str[i].flag;
+
+ *mtf_mask |= flags_str[i].flag;
+ found = true;
+ break;
+ }
+ }
+ if (!found)
return -1;
- } else {
- mtf_mask = 0xffffffff;
+
+ token = strtok(NULL, "/");
}
- addattr32(n, MAX_MSG, type, htonl(mtf));
- addattr32(n, MAX_MSG, mask_type, htonl(mtf_mask));
return 0;
}
@@ -433,6 +465,8 @@ static int flower_parse_opt(struct filter_util *qu, char *handle,
__be16 vlan_ethtype = 0;
__u8 ip_proto = 0xff;
__u32 flags = 0;
+ __u32 mtf = 0;
+ __u32 mtf_mask = 0;
if (handle) {
ret = get_u32(&t->tcm_handle, handle, 0);
@@ -462,14 +496,14 @@ static int flower_parse_opt(struct filter_util *qu, char *handle,
return -1;
}
addattr_l(n, MAX_MSG, TCA_FLOWER_CLASSID, &handle, 4);
- } else if (matches(*argv, "matching_flags") == 0) {
+ } else if (matches(*argv, "ip_flags") == 0) {
NEXT_ARG();
ret = flower_parse_matching_flags(*argv,
- TCA_FLOWER_KEY_FLAGS,
- TCA_FLOWER_KEY_FLAGS_MASK,
- n);
+ FLOWER_IP_FLAGS,
+ &mtf,
+ &mtf_mask);
if (ret < 0) {
- fprintf(stderr, "Illegal \"matching_flags\"\n");
+ fprintf(stderr, "Illegal \"ip_flags\"\n");
return -1;
}
} else if (matches(*argv, "skip_hw") == 0) {
@@ -723,6 +757,16 @@ static int flower_parse_opt(struct filter_util *qu, char *handle,
parse_done:
addattr32(n, MAX_MSG, TCA_FLOWER_FLAGS, flags);
+ if (mtf_mask) {
+ ret = addattr32(n, MAX_MSG, TCA_FLOWER_KEY_FLAGS, htonl(mtf));
+ if (ret)
+ return ret;
+
+ ret = addattr32(n, MAX_MSG, TCA_FLOWER_KEY_FLAGS_MASK, htonl(mtf_mask));
+ if (ret)
+ return ret;
+ }
+
ret = addattr16(n, MAX_MSG, TCA_FLOWER_KEY_ETH_TYPE, eth_type);
if (ret) {
fprintf(stderr, "Illegal \"eth_type\"(0x%x)\n",
@@ -828,14 +872,36 @@ static void flower_print_ip_proto(FILE *f, __u8 *p_ip_proto,
}
static void flower_print_matching_flags(FILE *f, char *name,
+ enum flower_matching_flags type,
struct rtattr *attr,
struct rtattr *mask_attr)
{
+ int i;
+ int count = 0;
+ __u32 mtf;
+ __u32 mtf_mask;
+
if (!mask_attr || RTA_PAYLOAD(mask_attr) != 4)
return;
- fprintf(f, "\n %s 0x%08x/0x%08x", name, ntohl(rta_getattr_u32(attr)),
- mask_attr ? ntohl(rta_getattr_u32(mask_attr)) : 0xffffffff);
+ mtf = ntohl(rta_getattr_u32(attr));
+ mtf_mask = ntohl(rta_getattr_u32(mask_attr));
+
+ for (i = 0; i < ARRAY_SIZE(flags_str); i++) {
+ if (type != flags_str[i].type)
+ continue;
+ if (mtf_mask & flags_str[i].flag) {
+ if (++count == 1)
+ fprintf(f, "\n %s ", name);
+ else
+ fprintf(f, "/");
+
+ if (mtf & flags_str[i].flag)
+ fprintf(f, "%s", flags_str[i].string);
+ else
+ fprintf(f, "no%s", flags_str[i].string);
+ }
+ }
}
static void flower_print_ip_addr(FILE *f, char *name, __be16 eth_type,
@@ -1034,7 +1100,8 @@ static int flower_print_opt(struct filter_util *qu, FILE *f,
flower_print_port(f, "enc_dst_port",
tb[TCA_FLOWER_KEY_ENC_UDP_DST_PORT]);
- flower_print_matching_flags(f, "matching_flags",
+ flower_print_matching_flags(f, "ip_flags",
+ FLOWER_IP_FLAGS,
tb[TCA_FLOWER_KEY_FLAGS],
tb[TCA_FLOWER_KEY_FLAGS_MASK]);
--
2.7.4
^ permalink raw reply related
* Re: [PATCH net-next v3 06/10] net: dsa: Migrate to device_find_class()
From: Greg KH @ 2017-01-19 14:28 UTC (permalink / raw)
To: Florian Fainelli
Cc: Andrew Lunn, Jason Cooper, Vivien Didelot, netdev, Russell King,
open list, Gregory Clement, David S. Miller,
moderated list:ARM SUB-ARCHITECTURES, Sebastian Hesselbarth
In-Reply-To: <912e6143-c67a-d909-2a6c-939de79efc5d@gmail.com>
On Mon, Jan 16, 2017 at 12:01:02PM -0800, Florian Fainelli wrote:
> On 01/15/2017 11:16 AM, Andrew Lunn wrote:
> >>> What exactly is the relationship between these devices (a ascii-art tree
> >>> or sysfs tree output might be nice) so I can try to understand what is
> >>> going on here.
> >
> > Hi Greg, Florian
> >
> > A few diagrams and trees which might help understand what is going on.
> >
> > The first diagram comes from the 2008 patch which added all this code:
> >
> > +-----------+ +-----------+
> > | | RGMII | |
> > | +-------+ +------ 1000baseT MDI ("WAN")
> > | | | 6-port +------ 1000baseT MDI ("LAN1")
> > | CPU | | ethernet +------ 1000baseT MDI ("LAN2")
> > | |MIImgmt| switch +------ 1000baseT MDI ("LAN3")
> > | +-------+ w/5 PHYs +------ 1000baseT MDI ("LAN4")
> > | | | |
> > +-----------+ +-----------+
> >
> > We have an ethernet switch and a host CPU. The switch is connected to
> > the CPU in two different ways. RGMII allows us to get Ethernet frames
> > from the CPU into the switch. MIImgmt, is the management bus normally
> > used for Ethernet PHYs, but Marvell switches also use it for Managing
> > switches.
> >
> > The diagram above is the simplest setup. You can have multiple
> > Ethernet switches, connected together via switch ports. Each switch
> > has its own MIImgmt connect to the CPU, but there is only one RGMII
> > link.
> >
> > When this code was designed back in 2008, it was decided to represent
> > this is a platform device, and it has a platform_data, which i have
> > slightly edited to keep it simple:
> >
> > struct dsa_platform_data {
> > /*
> > * Reference to a Linux network interface that connects
> > * to the root switch chip of the tree.
> > */
> > struct device *netdev;
This I think is the oddest thing, why do you need to have the "root
switch" here? You seem to have dropped the next value in this
structure:
struct net_device *of_netdev;
Isn't that the "real" net_device you need/want to get to here?
Or, when you set netdev, can't you also point to the "real" net_device
you want to later get to?
> > /*
> > * Info structs describing each of the switch chips
> > * connected via this network interface.
> > */
> > int nr_chips;
> > struct dsa_chip_data *chip;
> > };
> >
> > This netdev is the CPU side of the RGMII interface.
> >
> > Each switch has a dsa_chip_data, again edited:
> >
> > struct dsa_chip_data {
> > /*
> > * How to access the switch configuration registers.
> > */
> > struct device *host_dev;
> > int sw_addr;
> > ...
> > }
If each switch only has one dsa_chip_data, can't you point directly from
that structure back to the dsa_platform_device? Or is that what this
"host_dev" pointer is pointing to?
> > The host_dev is the CPU side of the MIImgmt, and we have the address
> > the switch is using on the bus.
"cpu side" means what? What 'struct device' is this? The host
controller? The platform device? Something else?
> > During probe of this platform device, we need to get from the
> > struct device *netdev to a struct net_device *dev.
Wait, what exactly is this "struct device *"? Who created it?
And why are you using a platform device? Shouldn't this be a custom
bus? I know people like to abuse platform devices, is that the case
here, or is it really driven only by a device tree representation?
Shouldn't you have a bus for RGMII devices? Is that the real problem
here, you don't have a representation for your RGMII "bus" with a
controller to bundle everything under (like a USB host controller, it
bridges from one bus to another).
> > So the code looks in the device net class to find the device
> >
> > | | | |-- f1074000.ethernet
> > | | | | |-- deferred_probe
> > | | | | |-- driver -> ../../../../../bus/platform/drivers/mvneta
> > | | | | |-- driver_override
> > | | | | |-- modalias
> > | | | | |-- net
> > | | | | | `-- eth1
> > | | | | | |-- addr_assign_type
> > | | | | | |-- address
> > | | | | | |-- addr_len
> > | | | | | |-- broadcast
> > | | | | | |-- carrier
> > | | | | | |-- carrier_changes
> > | | | | | |-- deferred_probe
> > | | | | | |-- device -> ../../../f1074000.ethernet
> >
> > and then use container_of() to get the net_device.
What 'struct device *' are you passing to container_of() here? The one
representing eth1? What call is this? And where are you trying to go
from there, to a peer? Why?
> > Similarly, the code needs to get from struct device *host_dev to a struct mii_bus *.
> >
> > | | | |-- f1072004.mdio
> > | | | | |-- deferred_probe
> > | | | | |-- driver -> ../../../../../bus/platform/drivers/orion-mdio
> > | | | | |-- driver_override
> > | | | | |-- mdio_bus
> > | | | | | `-- f1072004.mdio-mi
> > | | | | | |-- deferred_probe
> > | | | | | |-- device -> ../../../f1072004.mdio
Ok, this looks like the "peer" you want to get to from eth1, you want
f1072004.mdio-mi, right?
If so, why is eth1 not below f1072004.mdio-mi in the heirachy already?
Why is it up attached to the parent device, making this a "flat"
heirachy?
That's what is confusing about your functions, you are just walking all
of the "child" devices of a specific struct device, trying to find the
first random one that happens to belong to a specific 'bus' because you
are related to it.
Which is wrong, eth1 should be a child of your bus device, that way you
"know" how to get to it (it's your parent!).
So, I'm still confused, and still think this is all wrong, but feel free
to prove me wrong about this :)
thanks,
greg k-h
^ permalink raw reply
* [PATCH iproute2 net-next V5] tc: flower: Refactor matching flags to be more user friendly
From: Paul Blakey @ 2017-01-19 14:27 UTC (permalink / raw)
To: Stephen Hemminger, netdev
Cc: Jiri Pirko, Or Gerlitz, Roi Dayan, Jiri Benc, Simon Horman,
Paul Blakey
Instead of "magic numbers" we can now specify each flag
by name. Prefix of "no" (e.g nofrag) unsets the flag,
otherwise it wil be set.
Example:
# add a flower filter that will drop fragmented packets
tc filter add dev ens4f0 protocol ip parent ffff: \
flower \
src_mac e4:1d:2d:fd:8b:01 \
dst_mac e4:1d:2d:fd:8b:02 \
indev ens4f0 \
ip_flags frag \
action drop
# add a flower filter that will drop non-fragmented packets
tc filter add dev ens4f0 protocol ip parent ffff: \
flower \
src_mac e4:1d:2d:fd:8b:01 \
dst_mac e4:1d:2d:fd:8b:02 \
indev ens4f0 \
ip_flags nofrag \
action drop
Fixes: 22a8f019891c ('tc: flower: support matching flags')
Signed-off-by: Paul Blakey <paulb@mellanox.com>
Reviewed-by: Roi Dayan <roid@mellanox.com>
---
Hi,
Added a framework to add new flags more easily, such
as the upcoming tcp_flags (see kernel cls_flower), and other ip_flags.
Thanks,
Paul.
Changelog:
v5:
Fixed wrong use of strtok to skip old prefix.
v4:
Changed prefix in manpage as well.
v3:
Changed prefix to "no" instead of "no_".
v2:
Changed delimiter to "/" to avoid shell pipe errors.
man/man8/tc-flower.8 | 12 +++++-
tc/f_flower.c | 117 ++++++++++++++++++++++++++++++++++++++++-----------
2 files changed, 102 insertions(+), 27 deletions(-)
diff --git a/man/man8/tc-flower.8 b/man/man8/tc-flower.8
index 2dd2c5e..2bdd2ef 100644
--- a/man/man8/tc-flower.8
+++ b/man/man8/tc-flower.8
@@ -46,7 +46,9 @@ flower \- flow based traffic control filter
.BR enc_dst_ip " | " enc_src_ip " } { "
.IR ipv4_address " | " ipv6_address " } | "
.B enc_dst_port
-.IR port_number
+.IR port_number " | "
+.BR ip_flags
+.IR IP_FLAGS
.SH DESCRIPTION
The
.B flower
@@ -183,13 +185,19 @@ prefix length. If the prefix is missing, \fBtc\fR assumes a full-length
host match. Dst port
.I NUMBER
is a 16 bit UDP dst port.
+.TP
+.BI ip_flags " IP_FLAGS"
+.I IP_FLAGS
+may be either
+.BR frag " or " nofrag
+to match on fragmented packets or not respectively.
.SH NOTES
As stated above where applicable, matches of a certain layer implicitly depend
on the matches of the next lower layer. Precisely, layer one and two matches
(\fBindev\fR, \fBdst_mac\fR and \fBsrc_mac\fR)
have no dependency, layer three matches
(\fBip_proto\fR, \fBdst_ip\fR, \fBsrc_ip\fR, \fBarp_tip\fR, \fBarp_sip\fR,
-\fBarp_op\fR, \fBarp_tha\fR and \fBarp_sha\fR)
+\fBarp_op\fR, \fBarp_tha\fR, \fBarp_sha\fR and \fBip_flags\fR)
depend on the
.B protocol
option of tc filter, layer four port matches
diff --git a/tc/f_flower.c b/tc/f_flower.c
index d301db3..b42e529 100644
--- a/tc/f_flower.c
+++ b/tc/f_flower.c
@@ -24,6 +24,10 @@
#include "tc_util.h"
#include "rt_names.h"
+enum flower_matching_flags {
+ FLOWER_IP_FLAGS,
+};
+
enum flower_endpoint {
FLOWER_ENDPOINT_SRC,
FLOWER_ENDPOINT_DST
@@ -63,7 +67,7 @@ static void explain(void)
" enc_dst_ip [ IPV4-ADDR | IPV6-ADDR ] |\n"
" enc_src_ip [ IPV4-ADDR | IPV6-ADDR ] |\n"
" enc_key_id [ KEY-ID ] |\n"
- " matching_flags MATCHING-FLAGS | \n"
+ " ip_flags IP-FLAGS | \n"
" enc_dst_port [ port_number ] }\n"
" FILTERID := X:Y:Z\n"
" MASKED_LLADDR := { LLADDR | LLADDR/MASK | LLADDR/BITS }\n"
@@ -136,28 +140,56 @@ static int flower_parse_vlan_eth_type(char *str, __be16 eth_type, int type,
return 0;
}
-static int flower_parse_matching_flags(char *str, int type, int mask_type,
- struct nlmsghdr *n)
-{
- __u32 mtf, mtf_mask;
- char *c;
+struct flag_to_string {
+ int flag;
+ enum flower_matching_flags type;
+ char *string;
+};
- c = strchr(str, '/');
- if (c)
- *c = '\0';
+static struct flag_to_string flags_str[] = {
+ { TCA_FLOWER_KEY_FLAGS_IS_FRAGMENT, FLOWER_IP_FLAGS, "frag" },
+};
- if (get_u32(&mtf, str, 0))
- return -1;
+static int flower_parse_matching_flags(char *str,
+ enum flower_matching_flags type,
+ __u32 *mtf, __u32 *mtf_mask)
+{
+ char *token;
+ bool no;
+ bool found;
+ int i;
- if (c) {
- if (get_u32(&mtf_mask, ++c, 0))
+ token = strtok(str, "/");
+
+ while (token) {
+ if (!strncmp(token, "no", 2)) {
+ no = true;
+ token += 2;
+ } else
+ no = false;
+
+ found = false;
+ for (i = 0; i < ARRAY_SIZE(flags_str); i++) {
+ if (type != flags_str[i].type)
+ continue;
+
+ if (!strcmp(token, flags_str[i].string)) {
+ if (no)
+ *mtf &= ~flags_str[i].flag;
+ else
+ *mtf |= flags_str[i].flag;
+
+ *mtf_mask |= flags_str[i].flag;
+ found = true;
+ break;
+ }
+ }
+ if (!found)
return -1;
- } else {
- mtf_mask = 0xffffffff;
+
+ token = strtok(NULL, "/");
}
- addattr32(n, MAX_MSG, type, htonl(mtf));
- addattr32(n, MAX_MSG, mask_type, htonl(mtf_mask));
return 0;
}
@@ -433,6 +465,8 @@ static int flower_parse_opt(struct filter_util *qu, char *handle,
__be16 vlan_ethtype = 0;
__u8 ip_proto = 0xff;
__u32 flags = 0;
+ __u32 mtf = 0;
+ __u32 mtf_mask = 0;
if (handle) {
ret = get_u32(&t->tcm_handle, handle, 0);
@@ -462,14 +496,14 @@ static int flower_parse_opt(struct filter_util *qu, char *handle,
return -1;
}
addattr_l(n, MAX_MSG, TCA_FLOWER_CLASSID, &handle, 4);
- } else if (matches(*argv, "matching_flags") == 0) {
+ } else if (matches(*argv, "ip_flags") == 0) {
NEXT_ARG();
ret = flower_parse_matching_flags(*argv,
- TCA_FLOWER_KEY_FLAGS,
- TCA_FLOWER_KEY_FLAGS_MASK,
- n);
+ FLOWER_IP_FLAGS,
+ &mtf,
+ &mtf_mask);
if (ret < 0) {
- fprintf(stderr, "Illegal \"matching_flags\"\n");
+ fprintf(stderr, "Illegal \"ip_flags\"\n");
return -1;
}
} else if (matches(*argv, "skip_hw") == 0) {
@@ -723,6 +757,16 @@ static int flower_parse_opt(struct filter_util *qu, char *handle,
parse_done:
addattr32(n, MAX_MSG, TCA_FLOWER_FLAGS, flags);
+ if (mtf_mask) {
+ ret = addattr32(n, MAX_MSG, TCA_FLOWER_KEY_FLAGS, htonl(mtf));
+ if (ret)
+ return ret;
+
+ ret = addattr32(n, MAX_MSG, TCA_FLOWER_KEY_FLAGS_MASK, htonl(mtf_mask));
+ if (ret)
+ return ret;
+ }
+
ret = addattr16(n, MAX_MSG, TCA_FLOWER_KEY_ETH_TYPE, eth_type);
if (ret) {
fprintf(stderr, "Illegal \"eth_type\"(0x%x)\n",
@@ -828,14 +872,36 @@ static void flower_print_ip_proto(FILE *f, __u8 *p_ip_proto,
}
static void flower_print_matching_flags(FILE *f, char *name,
+ enum flower_matching_flags type,
struct rtattr *attr,
struct rtattr *mask_attr)
{
+ int i;
+ int count = 0;
+ __u32 mtf;
+ __u32 mtf_mask;
+
if (!mask_attr || RTA_PAYLOAD(mask_attr) != 4)
return;
- fprintf(f, "\n %s 0x%08x/0x%08x", name, ntohl(rta_getattr_u32(attr)),
- mask_attr ? ntohl(rta_getattr_u32(mask_attr)) : 0xffffffff);
+ mtf = ntohl(rta_getattr_u32(attr));
+ mtf_mask = ntohl(rta_getattr_u32(mask_attr));
+
+ for (i = 0; i < ARRAY_SIZE(flags_str); i++) {
+ if (type != flags_str[i].type)
+ continue;
+ if (mtf_mask & flags_str[i].flag) {
+ if (++count == 1)
+ fprintf(f, "\n %s ", name);
+ else
+ fprintf(f, "/");
+
+ if (mtf & flags_str[i].flag)
+ fprintf(f, "%s", flags_str[i].string);
+ else
+ fprintf(f, "no%s", flags_str[i].string);
+ }
+ }
}
static void flower_print_ip_addr(FILE *f, char *name, __be16 eth_type,
@@ -1034,7 +1100,8 @@ static int flower_print_opt(struct filter_util *qu, FILE *f,
flower_print_port(f, "enc_dst_port",
tb[TCA_FLOWER_KEY_ENC_UDP_DST_PORT]);
- flower_print_matching_flags(f, "matching_flags",
+ flower_print_matching_flags(f, "ip_flags",
+ FLOWER_IP_FLAGS,
tb[TCA_FLOWER_KEY_FLAGS],
tb[TCA_FLOWER_KEY_FLAGS_MASK]);
--
2.7.4
^ permalink raw reply related
* Re: [PATCH] rtlwifi: rtl8192x: Enabling and disabling hardware interrupts after enabling local irq flags
From: Lino Sanfilippo @ 2017-01-19 14:35 UTC (permalink / raw)
To: Bharat Kumar Gogada
Cc: Larry.Finger, chaoming_li, linux-wireless, linux-kernel, kvalo,
netdev, rgummal, Bharat Kumar Gogada
In-Reply-To: <1484820854-16719-1-git-send-email-bharatku@xilinx.com>
Hi,
altek/rtlwifi/rtl8192ce/hw.c b/drivers/net/wireless/realtek/rtlwifi/rtl8192ce/hw.c
> index a47be73..143766c4 100644
> --- a/drivers/net/wireless/realtek/rtlwifi/rtl8192ce/hw.c
> +++ b/drivers/net/wireless/realtek/rtlwifi/rtl8192ce/hw.c
> @@ -1306,9 +1306,9 @@ void rtl92ce_enable_interrupt(struct ieee80211_hw *hw)
> struct rtl_priv *rtlpriv = rtl_priv(hw);
> struct rtl_pci *rtlpci = rtl_pcidev(rtl_pcipriv(hw));
>
> + rtlpci->irq_enabled = true;
> rtl_write_dword(rtlpriv, REG_HIMR, rtlpci->irq_mask[0] & 0xFFFFFFFF);
> rtl_write_dword(rtlpriv, REG_HIMRE, rtlpci->irq_mask[1] & 0xFFFFFFFF);
> - rtlpci->irq_enabled = true;
> }
>
> void rtl92ce_disable_interrupt(struct ieee80211_hw *hw)
> @@ -1316,9 +1316,9 @@ void rtl92ce_disable_interrupt(struct ieee80211_hw *hw)
> struct rtl_priv *rtlpriv = rtl_priv(hw);
> struct rtl_pci *rtlpci = rtl_pcidev(rtl_pcipriv(hw));
>
> + rtlpci->irq_enabled = false;
> rtl_write_dword(rtlpriv, REG_HIMR, IMR8190_DISABLED);
> rtl_write_dword(rtlpriv, REG_HIMRE, IMR8190_DISABLED);
> - rtlpci->irq_enabled = false;
> }
>
AFAIK you also have to use memory barriers here to ensure that
the concerning instructions are not reordered, and both irq handler
and process have a consistent perception of irq_enabled, e.g:
rtlpci->irq_enabled = true;
smp_wmb();
rtl_write_dword(rtlpriv, REG_HIMR, rtlpci->irq_mask[0] & 0xFFFFFFFF);
and in the irq handler
if (rtlpci->irq_enabled == 0) {
smp_rmb();
return ret;
}
Regards,
Lino
^ permalink raw reply
* Re: [PATCH iproute2 net-next V5] tc: flower: Refactor matching flags to be more user friendly
From: Jiri Benc @ 2017-01-19 14:48 UTC (permalink / raw)
To: Paul Blakey
Cc: Stephen Hemminger, netdev, Jiri Pirko, Or Gerlitz, Roi Dayan,
Simon Horman
In-Reply-To: <1484836073-47019-1-git-send-email-paulb@mellanox.com>
On Thu, 19 Jan 2017 16:27:53 +0200, Paul Blakey wrote:
> Instead of "magic numbers" we can now specify each flag
> by name. Prefix of "no" (e.g nofrag) unsets the flag,
> otherwise it wil be set.
>
> Example:
> # add a flower filter that will drop fragmented packets
> tc filter add dev ens4f0 protocol ip parent ffff: \
> flower \
> src_mac e4:1d:2d:fd:8b:01 \
> dst_mac e4:1d:2d:fd:8b:02 \
> indev ens4f0 \
> ip_flags frag \
> action drop
>
> # add a flower filter that will drop non-fragmented packets
> tc filter add dev ens4f0 protocol ip parent ffff: \
> flower \
> src_mac e4:1d:2d:fd:8b:01 \
> dst_mac e4:1d:2d:fd:8b:02 \
> indev ens4f0 \
> ip_flags nofrag \
> action drop
>
> Fixes: 22a8f019891c ('tc: flower: support matching flags')
> Signed-off-by: Paul Blakey <paulb@mellanox.com>
> Reviewed-by: Roi Dayan <roid@mellanox.com>
Reviewed-by: Jiri Benc <jbenc@redhat.com>
^ permalink raw reply
* Re: [PATCH net-next v3 06/10] net: dsa: Migrate to device_find_class()
From: Andrew Lunn @ 2017-01-19 14:53 UTC (permalink / raw)
To: Greg KH
Cc: Florian Fainelli, Jason Cooper, Vivien Didelot, netdev,
Russell King, open list, Gregory Clement, David S. Miller,
moderated list:ARM SUB-ARCHITECTURES, Sebastian Hesselbarth
In-Reply-To: <20170119142820.GA494@kroah.com>
> > > struct dsa_platform_data {
> > > /*
> > > * Reference to a Linux network interface that connects
> > > * to the root switch chip of the tree.
> > > */
> > > struct device *netdev;
>
> This I think is the oddest thing, why do you need to have the "root
> switch" here? You seem to have dropped the next value in this
> structure:
> struct net_device *of_netdev;
We are implementing platform_data for devices which don't support
device tree. When using OF, we don't have any of these issues. We can
go straight to the device.
It is a bit convoluted, but look at
arch/arm/mach-orion5x/rd88f5181l-ge-setup.c. It defines the start of
the dsa_platform_data in that file. It then gets passed through
common.c: orion5x_eth_switch_init() to
arch/arm/plat-orion/common.c:orion_ge00_switch_init() :
void __init orion_ge00_switch_init(struct dsa_platform_data *d)
{
int i;
d->netdev = &orion_ge00.dev;
for (i = 0; i < d->nr_chips; i++)
d->chip[i].host_dev = &orion_ge_mvmdio.dev;
platform_device_register_data(NULL, "dsa", 0, d, sizeof(d));
}
Where we have
static struct platform_device orion_ge00 = {
.name = MV643XX_ETH_NAME,
.id = 0,
.num_resources = 1,
.resource = orion_ge00_resources,
.dev = {
.coherent_dma_mask = DMA_BIT_MASK(32),
},
};
So this is the platform device for the Ethernet device. We cannot go
to the net_device, because it does not exist until this Ethernet
platform device is instantiated.
> Shouldn't you have a bus for RGMII devices? Is that the real problem
> here, you don't have a representation for your RGMII "bus" with a
> controller to bundle everything under (like a USB host controller, it
> bridges from one bus to another).
RGMII is not a bus. It is a point to point link. Normally, it is
between the Ethernet MAC and the Ethernet PHY. But you can also have
it between an Ethernet MAC and another Ethernet MAC. I'm not sure
describing this is a bus would be practical. It would mean every
ethernet driver also becomes a bus driver! Every Ethernet PHY would
become a bus device. That is a huge change, for a few legacy boards
which are not getting converted to device tree.
> If so, why is eth1 not below f1072004.mdio-mi in the heirachy already?
See the initial diagram above. The switch has two parents. It hangs of
an MDIO bus, and you would like to make RGMII also a bus. Can the
device model handle that? I thought it was a tree, not a graph?
Andrew
^ permalink raw reply
* Re: [PATCH iproute2 net-next V2] tc: flower: Refactor matching flags to be more user friendly
From: Jiri Benc @ 2017-01-19 14:04 UTC (permalink / raw)
To: Paul Blakey
Cc: Stephen Hemminger, netdev, Jiri Pirko, Or Gerlitz, Roi Dayan,
Simon Horman
In-Reply-To: <1484828277-29178-1-git-send-email-paulb@mellanox.com>
On Thu, 19 Jan 2017 14:17:57 +0200, Paul Blakey wrote:
> Instead of "magic numbers" we can now specify each flag
> by name. Prefix of "no_" (e.g no_frag) unsets the flag,
> otherwise it wil be set.
Other parts of the tc tool use the "no" prefix (without the
underscore), such as "nofrag" in u32 and pedit, "noecn" in fq_codel and
pie, etc. We should follow the suit here.
Looks good to me otherwise, thanks a lot for doing the work!
Jiri
^ permalink raw reply
* Re: [PATCH net-next V4] tc: flower: Refactor matching flags to be more user friendly
From: Paul Blakey @ 2017-01-19 14:29 UTC (permalink / raw)
To: Jiri Benc
Cc: paulb, Stephen Hemminger, netdev, Jiri Pirko, Or Gerlitz,
Roi Dayan, Simon Horman
In-Reply-To: <20170119152218.61aa2fc4@griffin>
On 19/01/2017 16:22, Jiri Benc wrote:
> On Thu, 19 Jan 2017 16:17:48 +0200, Paul Blakey wrote:
>> + while (token) {
>> + if (!strncmp(token, "no", 2)) {
>> + no = true;
>> + token = strchr(token, '_') + 1;
>
> This seems to still assume that "no" is followed by an underscore.
> What about a simple token += 2?
>
> Jiri
>
Thanks again, v5 is sent.
^ permalink raw reply
* Re: [PATCH iproute2 net-next V5] tc: flower: Refactor matching flags to be more user friendly
From: Or Gerlitz @ 2017-01-19 14:53 UTC (permalink / raw)
To: Simon Horman
Cc: Paul Blakey, Stephen Hemminger, netdev, Jiri Pirko, Roi Dayan,
Jiri Benc
In-Reply-To: <1484836073-47019-1-git-send-email-paulb@mellanox.com>
On 1/19/2017 4:27 PM, Paul Blakey wrote:
> Hi, added a framework to add new flags more easily, such
> as the upcoming tcp_flags (see kernel cls_flower), and other ip_flags.
FWIW, just wanted to make a note that I have the patches for tcp_flags
under the works, so if someone needs them urgently send me email, want
to avoid wasting your time implementing this..
Or.
^ permalink raw reply
* [PATCH net-next] net: caif: Remove unused stats member from struct chnl_net
From: Tobias Klauser @ 2017-01-19 15:04 UTC (permalink / raw)
To: Dmitry Tarnyagin, David S. Miller; +Cc: netdev
The stats member of struct chnl_net is used nowhere in the code, so it
might as well be removed.
Signed-off-by: Tobias Klauser <tklauser@distanz.ch>
---
net/caif/chnl_net.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/net/caif/chnl_net.c b/net/caif/chnl_net.c
index 3408ed51b611..1816fc9f1ee7 100644
--- a/net/caif/chnl_net.c
+++ b/net/caif/chnl_net.c
@@ -44,7 +44,6 @@ enum caif_states {
struct chnl_net {
struct cflayer chnl;
- struct net_device_stats stats;
struct caif_connect_request conn_req;
struct list_head list_field;
struct net_device *netdev;
--
2.11.0
^ permalink raw reply related
* Re: Initializing MAC address at run-time
From: Mason @ 2017-01-19 15:31 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: Mark Rutland, DT, Arnd Bergmann, Kevin Hilman, netdev,
Thibaud Cornic, Linux ARM
In-Reply-To: <20170118185435.fen6np7lydzgxxek@pengutronix.de>
Hello Uwe,
On 18/01/2017 19:54, Uwe Kleine-König wrote:
> On Wed, Jan 18, 2017 at 03:03:57PM +0100, Mason wrote:
>
>> When my system boots up, eth0 is given a seemingly random MAC address.
>>
>> [ 0.950734] nb8800 26000.ethernet eth0: MAC address ba:de:d6:38:b8:38
>> [ 0.957334] nb8800 26000.ethernet eth0: MAC address 6e:f1:48:de:d6:c4
>>
>>
>> The DT node for eth0 is:
>>
>> eth0: ethernet@26000 {
>> compatible = "sigma,smp8734-ethernet";
>> reg = <0x26000 0x800>;
>> interrupts = <38 IRQ_TYPE_LEVEL_HIGH>;
>> clocks = <&clkgen SYS_CLK>;
>> };
>>
>> Documentation/devicetree/bindings/net/ethernet.txt mentions
>> - local-mac-address: array of 6 bytes, specifies the MAC address that was
>> assigned to the network device;
>>
>> And indeed, if I define this property, eth0 ends up with the MAC address
>> I specify in the device tree. But of course, I don't want all my boards
>> to share the same MAC address. Every interface has a unique MAC address.
>>
>> In fact, the boot loader (not Uboot, a custom non-DT boot loader) stores
>> the MAC address somewhere in MMIO space, in some weird custom format.
>
> Where does your machine get the dtb from? You write it to the boot
> medium at a certain point of time I assume.
The DTB is appended to the kernel uImage.
(It's never written to the board's storage.)
> So AFAICT you have the following options (in no particular order):
>
> a) Describe in the dtb how to find out how the MAC address is stored
> (already pointed out Mark and Robin)
> b) Make your bootloader dt aware and let it provide the
> local-mac-address property.
Do you agree that such boot loader would execute code that is roughly
identical to the one posted for illustration purposes?
1. find the MAC address to use for eth0
2. find the eth0 node in the DT
3. insert the right prop in the eth0 node
In which case, it seems a waste to add the DT library to the boot
loader, when the operation can be done in Linux, which requires the
DT library anyway. (Additionally, adding DT support to some custom
legacy boot loader might be a complex task.)
> c) Adapt the dtb before it is written to the boot medium.
This is not applicable, as the DTB is not written to the board.
> d) Let the bootloader configure the device and teach the driver to pick
> up the mac from the device's address space.
I'm not sure what you call "the device" ?
The local RAM where the MAC is stored? the eth controller?
> e) Accept that the mac address is random during development, and make
> Userspace configure the MAC address, which is early enough for
> production use.
During development, some devs configure the DHCP server to provide
a specific uImage and/or rootfs to their board, based on the MAC
address. This scheme would fall apart with a random MAC.
> Not sure d) is considered ok today, but some drivers have this feature.
> I'd say b) is the best choice.
In my mind, doing it early in Linux is similar in spirit to doing it
at the boot loader stage, in that it's neatly separated from the rest
of the setup.
>> I need to do something similar with the NAND partitions. The boot loader
>> stores the partition offsets somewhere, and I need to pass this info
>> to the NAND framework, so I assumed that inserting the corresponding
>> properties at run-time was the correct way to do it.
>
> The list of options here is similar to the list above. d) doesn't work,
> but instead you can pass the partitioning on the kernel commandline.
Yes, I will test the command line approach. Thanks.
Regards.
^ permalink raw reply
* Re: [PATCH] net: ethtool: avoid allocation failure for dump_regs
From: John W. Linville @ 2017-01-19 14:15 UTC (permalink / raw)
To: David Arcari; +Cc: David Miller, netdev
In-Reply-To: <e88b01a6-dae8-a97e-9a8b-8aab030ad099@redhat.com>
On Thu, Jan 19, 2017 at 07:35:22AM -0500, David Arcari wrote:
> On 01/18/2017 11:45 AM, David Miller wrote:
> > From: David Arcari <darcari@redhat.com>
> > Date: Wed, 18 Jan 2017 08:34:05 -0500
> >
> >> If the user executes 'ethtool -d' for an interface and the associated
> >> get_regs_len() function returns 0, the user will see a call trace from
> >> the vmalloc() call in ethtool_get_regs(). This patch modifies
> >> ethtool_get_regs() to avoid the call to vmalloc when the size is zero.
> >>
> >> Signed-off-by: David Arcari <darcari@redhat.com>
> > I think when the driver indicates this, it is equivalent to saying that
> > the operation isn't supported.
> >
> > Also, this guards us against ->get_regs() methods that don't handle
> > zero length requests properly. I see many which are going to do
> > really terrible things in that situation.
> >
> > Therefore, if get_regs_len() returns zero, treat it the safe as if the
> > ethtool operations were NULL.
> >
> > Thanks.
>
> That was actually the fix that I was originally considering, but it
> turns out
> there is a problem with it.
>
> I found that the vmalloc error was occurring because
> ieee80211_get_regs_len() in
> net/mac80211/ethtool.c was returning zero. The ieee80211_get_regs in
> the same
> file returns the hw version. It turns out that this information is used
> by the
> at76c50x-usb driver in the user space ethtool to report which HW variant
> is in
> use. Returning an error when regs_len() returns zero would break this
> functionality.
>
> -Dave
I'm responsible for this mess. The original idea was for various
mac80211-based drivers to override the ethtool operation and provide
their own dump operation, but the mac80211 crowd never embraced
the idea.
In the meantime, I added the default implementation which just
passed-up wdev->wiphy->hw_version as the version info for a 0-length
register dump. I then implemented a driver-specific regiser dump
handler for userland ethtool that would interpret the hardware version
information for the at76c50x-usb driver.
So the net of it is, if we treat a return of 0 from get_regs_len()
as "not supported", we break this one driver-specific feature for
userland ethtool. Realistically, there are probably very few users
to care. But I can't guarantee that the number is zero.
Possible solutions:
-- break userland ethtool for at76c50x-usb
-- avoid 0-len allocation attempt (David Arcari's patch)
-- make allocator accept a 0 length value w/o oops'ing
-- change mac8011 code to return non-zero from get_regs_len()
Thoughts? The last option holds a certain attraction, but I'm not
sure how to make it useful...?
John
--
John W. Linville Someday the world will need a hero, and you
linville@tuxdriver.com might be all we have. Be ready.
^ permalink raw reply
* RE: [PATCH iproute2 v4 3/4] ifstat: Add 64 bits based stats to extended statistics
From: Nogah Frankel @ 2017-01-19 15:21 UTC (permalink / raw)
To: Stephen Hemminger
Cc: netdev@vger.kernel.org, roszenrami@gmail.com,
roopa@cumulusnetworks.com, Jiri Pirko, Ido Schimmel, Elad Raz,
Yotam Gigi, Or Gerlitz
In-Reply-To: <20170112174335.1bd391c2@xeon-e3>
> -----Original Message-----
> From: Nogah Frankel
> Sent: Sunday, January 15, 2017 3:55 PM
> To: 'Stephen Hemminger' <stephen@networkplumber.org>
> Cc: netdev@vger.kernel.org; roszenrami@gmail.com; roopa@cumulusnetworks.com; Jiri
> Pirko <jiri@mellanox.com>; Ido Schimmel <idosch@mellanox.com>; Elad Raz
> <eladr@mellanox.com>; Yotam Gigi <yotamg@mellanox.com>; Or Gerlitz
> <ogerlitz@mellanox.com>
> Subject: RE: [PATCH iproute2 v4 3/4] ifstat: Add 64 bits based stats to extended statistics
>
>
>
> > -----Original Message-----
> > From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> > Sent: Friday, January 13, 2017 3:44 AM
> > To: Nogah Frankel <nogahf@mellanox.com>
> > Cc: netdev@vger.kernel.org; roszenrami@gmail.com; roopa@cumulusnetworks.com;
> Jiri
> > Pirko <jiri@mellanox.com>; Ido Schimmel <idosch@mellanox.com>; Elad Raz
> > <eladr@mellanox.com>; Yotam Gigi <yotamg@mellanox.com>; Or Gerlitz
> > <ogerlitz@mellanox.com>
> > Subject: Re: [PATCH iproute2 v4 3/4] ifstat: Add 64 bits based stats to extended statistics
> >
> > On Thu, 12 Jan 2017 15:49:50 +0200
> > Nogah Frankel <nogahf@mellanox.com> wrote:
> >
> > > The default stats for ifstat are 32 bits based.
> > > The kernel supports 64 bits based stats. (They are returned in struct
> > > rtnl_link_stats64 which is an exact copy of struct rtnl_link_stats, in
> > > which the "normal" stats are returned, but with fields of u64 instead of
> > > u32). This patch adds them as an extended stats.
> > >
> > > It is read with filter type IFLA_STATS_LINK_64 and no sub type.
> > >
> > > It is under the name 64bits
> > > (or any shorten of it as "64")
> > >
> > > For example:
> > > ifstat -x 64bit
> > >
> > > Signed-off-by: Nogah Frankel <nogahf@mellanox.com>
> > > Reviewed-by: Jiri Pirko <jiri@mellanox.com>
> >
> > Other commands (like ip link) always use the 64 bit statistics if available
> > from the device. I see no reason that ifstat needs to be different.
> >
>
> Do you mean to change the default ifstat results to be 64 bits based?
> I tried it in the first version, but Roopa commented that it was not a good idea.
> She said they tried it in the past and it caused backward compatibilities problems.
> (Or maybe I didn't understand correctly)
So, can I leave the default ifstat results to be 32 bits based, for the time being?
^ permalink raw reply
* Re: Setting link down or up in software
From: Mason @ 2017-01-19 15:40 UTC (permalink / raw)
To: Zefir Kurtisi, netdev
Cc: Mans Rullgard, Florian Fainelli, Andrew Lunn, Thibaud Cornic
In-Reply-To: <dc6378d7-d370-c893-63b7-c21cd7ef76fd@neratec.com>
On 18/01/2017 11:29, Zefir Kurtisi wrote:
> On 01/13/2017 06:35 PM, Mason wrote:
>> On 13/01/2017 17:28, Zefir Kurtisi wrote:
>>
>>> As for your specific problem: since I fought myself with the PHY/ETH subsystems
>>> over the past months, I might remember something relevant to your issue. Could you
>>> give some more info on your setup (PHY driver, opmode (SGMII, RGMII, etc.), ETH).
>>
>> Hello Zefir,
>>
>> My boards are using these drivers:
>>
>> http://lxr.free-electrons.com/source/drivers/net/ethernet/aurora/nb8800.c
>> http://lxr.free-electrons.com/source/drivers/net/phy/at803x.c
>>
>> The relevant device tree nodes are:
>>
>> eth0: ethernet@26000 {
>> compatible = "sigma,smp8734-ethernet";
>> reg = <0x26000 0x800>;
>> interrupts = <38 IRQ_TYPE_LEVEL_HIGH>;
>> clocks = <&clkgen SYS_CLK>;
>> };
>>
>> ð0 {
>> phy-connection-type = "rgmii";
>> phy-handle = <ð0_phy>;
>> #address-cells = <1>;
>> #size-cells = <0>;
>>
>> /* Atheros AR8035 */
>> eth0_phy: ethernet-phy@4 {
>> compatible = "ethernet-phy-id004d.d072",
>> "ethernet-phy-ieee802.3-c22";
>> interrupts = <37 IRQ_TYPE_EDGE_RISING>;
>> reg = <4>;
>> };
>> };
>>
>> If I comment the PHY "interrupts" property, then the PHY framework
>> falls back to polling.
>>
>> Am I forgetting important information?
>
> In our system we attach the at8031 over SGMII to the gianfar (Freescale eTSEC) and
> to fibre optics transceivers, which operate in fixed speeds. Getting this setup to
> work reliably was challenging for various reasons, maybe worth to note
>
> 1) fixed SGMII speed not working: link is up on both ends, but no data is passed
> 2) known issue with SGMII link not completing autonegotiation correctly, see [1]
> 3) once autoneg is started or chip is reset, MII_CTRL1000 can not be written to
> until autoneg is completed => breaks phy state machine when the driver loads with
> unplugged cable and tries to set fixed speed
>
> Unless you are using fixed speed links in your setup, none of those should affect
> it. My experience with at8031 attached to RGMII is that it is genphy compliant,
> therefore I would a) disable interrupts, and b) prevent loading at803x.ko and try
> the genphy instead. Yours is an at8035, results may vary.
>
> [1] https://www.spinics.net/lists/netdev/msg400804.html
Hello Zefir,
Thanks for having a look at my issue.
I must confess that I'm not up to speed on the different *MIIs
(MII, RMII, GMII, RGMII, SGMII, etc)
One thing I find confusing... If the 8035 could work with the generic
PHY driver, what's the point of the at803x.ko driver?
Regards.
^ permalink raw reply
* Re: [PATCH v2 net-next] phy: increase size of bus_id and MII_BUS_ID_SIZE
From: Andrew Lunn @ 2017-01-19 14:25 UTC (permalink / raw)
To: Volodymyr Bendiuga
Cc: f.fainelli, netdev, volodymyr.bendiuga, Magnus Öberg
In-Reply-To: <1484826415-31631-1-git-send-email-volodymyr.bendiuga@gmail.com>
On Thu, Jan 19, 2017 at 12:46:55PM +0100, Volodymyr Bendiuga wrote:
> Some bus names are pretty long and do not fit into
> 20 chars, therefore bus_id size is increased together
> with MII_BUS_ID_SIZE to host larger names.
>
> Signed-off-by: Volodymyr Bendiuga <volodymyr.bendiuga@gmail.com>
> Signed-off-by: Magnus Öberg <magnus.oberg@westermo.se>
> ---
> include/linux/phy.h | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/include/linux/phy.h b/include/linux/phy.h
> index f7d95f6..f53f0cd 100644
> --- a/include/linux/phy.h
> +++ b/include/linux/phy.h
> @@ -162,7 +162,7 @@ static inline const char *phy_modes(phy_interface_t interface)
> * Need to be a little smaller than phydev->dev.bus_id to leave room
> * for the ":%02x"
It would also be good to fixup this comment. dev.bus_id does not seem
to exist any more. I think it got replaced by dev->kobj.name, which is
now a char * rather than an array. I think the comment can be removed.
> */
> -#define MII_BUS_ID_SIZE (20 - 3)
> +#define MII_BUS_ID_SIZE (64 - 3)
>
> /* Or MII_ADDR_C45 into regnum for read/write on mii_bus to enable the 21 bit
> IEEE 802.3ae clause 45 addressing mode used by 10GIGE phy chips. */
> @@ -632,7 +632,7 @@ struct phy_driver {
> /* A Structure for boards to register fixups with the PHY Lib */
> struct phy_fixup {
> struct list_head list;
> - char bus_id[20];
> + char bus_id[64];
[MII_BUS_ID_SIZE + 3] would be better.
Andrew
^ permalink raw reply
* Re: [PATCH v2 net-next] net:add one common config ARCH_WANT_RELAX_ORDER to support relax ordering.
From: Alexander Duyck @ 2017-01-19 15:54 UTC (permalink / raw)
To: David Laight
Cc: David Miller, maowenan@huawei.com, netdev@vger.kernel.org,
jeffrey.t.kirsher@intel.com
In-Reply-To: <063D6719AE5E284EB5DD2968C1650D6DB0267C5A@AcuExch.aculab.com>
On Thu, Jan 19, 2017 at 3:43 AM, David Laight <David.Laight@aculab.com> wrote:
> From: Alexander Duyck
>> Sent: 18 January 2017 17:25
>> On Wed, Jan 18, 2017 at 8:22 AM, David Laight <David.Laight@aculab.com> wrote:
>> > From: David Miller
>> >> Sent: 17 January 2017 19:16
>> >> > Relax ordering(RO) is one feature of 82599 NIC, to enable this feature can
>> >> > enhance the performance for some cpu architecure, such as SPARC and so on.
>> >> > Currently it only supports one special cpu architecture(SPARC) in 82599
>> >> > driver to enable RO feature, this is not very common for other cpu architecture
>> >> > which really needs RO feature.
>> >> > This patch add one common config CONFIG_ARCH_WANT_RELAX_ORDER to set RO feature,
>> >> > and should define CONFIG_ARCH_WANT_RELAX_ORDER in sparc Kconfig firstly.
>> >> >
>> >> > Signed-off-by: Mao Wenan <maowenan@huawei.com>
>> >>
>> >> Since no-one has reviewed this patch, and I do not feel comfortable with applying
>> >> it without such review, I am tossing this patch.
>> >>
>> >> If someone eventually reviews it, repost this patch.
>> >
>> > Having re-read parts of the PCIe spec I think I'd like someone to
>> > explain exactly which transfers are affected by the 'relaxed ordering'
>> > bit and why any re-ordered transactions aren't a problem.
>> >
>> > In particular I believe RO allows the write to update the receive
>> > descriptor ring to overtake a write of receive packet data.
>> > That could lead to the network stack processing a receive frame
>> > before it has actually been written.
>> >
>> > David
>> >
>>
>> The Relaxed Ordering attribute doesn't get applied across the board.
>> It ends up being limited to a subset of the transactions if I recall
>> correctly. In this case it is the Tx descriptor write back, and the
>> Rx data write back. We don't apply the RO bit to any other
>> transactions.
>>
>> In the case of Tx descriptor there is no harm in allowing it to be
>> reordered because we only really read the DD bit so we don't care
>> about the ordering of the write back. In the case of the Rx data the
>> Rx descriptor essentially acts as a flush since it is sent without the
>> RO bit set. So all the writes before it must be completed before the
>> Rx descriptor write back.
>
> In which case why not set it unconditionally for all architectures?
>
> I'm surprised (I often am) that allowing those re-orderings makes
> any significant difference.
> Unfortunately you need a PCIe analyser to see what is really happening
> and they don't come cheap.
>
> What I do vaguely remember is that some hosts don't always implement
> the 'normal' re-ordering of reads and read completions.
> Re-ordering of reads allows descriptor reads to overtake transmit
> traffic which is likely to make a difference.
I think part of the issue, at least in the case of SPARC, is that the
handling of the memory writes in the PCIe root complex is impacted by
the RO attribute. On the bus itself it doesn't matter much, but at
the root complex it can become expensive to have to wait on a partial
write to complete while there are other writes pending. This is why
the IOMMU for SPARC now has a WEAK_ORDERING attribute you can add so
that it can write the data in whatever order it wants in relation to
other writes in that region.
- Alex
^ 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