Netdev List
 help / color / mirror / Atom feed
* Re: [PATCH v3 2/2] netfilter: Create revision 2 of xt_hashlimit to support higher pps rates
From: Vishwanath Pai @ 2016-09-27  7:35 UTC (permalink / raw)
  To: Liping Zhang, Pablo Neira Ayuso
  Cc: Vishwanath Pai, Patrick McHardy, Jozsef Kadlecsik, Josh Hunt,
	netfilter-devel, coreteam, netdev
In-Reply-To: <CAML_gOdcrnmuHkxA1xAb2HDXHpo+=+sw+oJBfKXF3ijApsjTqg@mail.gmail.com>

On Tue, Sep 27, 2016 at 12:15 AM, Liping Zhang <zlpnobody@gmail.com> wrote:
> Hi Vishwanath,
>
> 2016-09-23 0:43 GMT+08:00 Vishwanath Pai <vpai@akamai.com>:
>>
>>  /* Precision saver. */
>> -static u32 user2credits(u32 user)
>> +static u64 user2credits(u64 user, int revision)
>>  {
>> -       /* If multiplying would overflow... */
>> -       if (user > 0xFFFFFFFF / (HZ*CREDITS_PER_JIFFY_v1))
>> -               /* Divide first. */
>> -               return (user / XT_HASHLIMIT_SCALE_v1) *\
>> -                                       HZ * CREDITS_PER_JIFFY_v1;
>> +       if (revision == 1) {
>> +               /* If multiplying would overflow... */
>> +               if (user > 0xFFFFFFFF / (HZ*CREDITS_PER_JIFFY_v1))
>> +                       /* Divide first. */
>> +                       return (user / XT_HASHLIMIT_SCALE_v1) *\
>> +                                               HZ * CREDITS_PER_JIFFY_v1;
>> +
>> +               return (user * HZ * CREDITS_PER_JIFFY_v1) \
>> +                                               / XT_HASHLIMIT_SCALE_v1;
>> +       } else {
>> +               if (user > 0xFFFFFFFFFFFFFFFF / (HZ*CREDITS_PER_JIFFY))
>> +                       return (user / XT_HASHLIMIT_SCALE) *\
>> +                                               HZ * CREDITS_PER_JIFFY;
>>
>> -       return (user * HZ * CREDITS_PER_JIFFY_v1) / XT_HASHLIMIT_SCALE_v1;
>> +               return (user * HZ * CREDITS_PER_JIFFY) / XT_HASHLIMIT_SCALE;
>> +       }
>>  }
>>
>
> In my memory, 64-bit division operation should be replaced by
> div_u64 or div64_u64, otherwise on some 32-bit architecture
> systems, link error will happen. Something like this:
> ... undefined reference to `__udivdi3'.

I did not know that, thanks for pointing it out. I will send a patch
to fix this.

-Vishwanath

^ permalink raw reply

* Re: [PATCH resend] sh_eth: add R8A7743/5 support
From: Geert Uytterhoeven @ 2016-09-27  7:35 UTC (permalink / raw)
  To: Sergei Shtylyov
  Cc: netdev@vger.kernel.org, Linux-Renesas, Rob Herring, Mark Rutland,
	devicetree@vger.kernel.org
In-Reply-To: <1654835.SEcvPVx4OQ@wasted.cogentembedded.com>

Hi Sergei,

On Tue, Sep 27, 2016 at 12:23 AM, Sergei Shtylyov
<sergei.shtylyov@cogentembedded.com> wrote:
> Add support for the first two members of the Renesas RZ/G family, RZ/G1M/E
> (also known as  R8A7743/5). The Ether core is the same as in the R-Car gen2
> SoCs, so will share the code/data with them...
>
> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>

> --- net-next.orig/drivers/net/ethernet/renesas/Kconfig
> +++ net-next/drivers/net/ethernet/renesas/Kconfig
> @@ -27,7 +27,7 @@ config SH_ETH
>           Renesas SuperH Ethernet device driver.
>           This driver supporting CPUs are:
>                 - SH7619, SH7710, SH7712, SH7724, SH7734, SH7763, SH7757,
> -                 R8A7740, R8A777x and R8A779x.
> +                 R8A7740, R8A774x, R8A777x and R8A779x.

Surely "R8A7740" is covered by "R8A774x"? :-)
However, the "x" is not a real wildcard (also for '7x and '9x), as the driver
doesn't support all possible values of "x".

Apart from that:
Acked-by: Geert Uytterhoeven <geert+renesas@glider.be>

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

^ permalink raw reply

* [PATCH] Fix link error in 32bit arch because of 64bit division
From: Vishwanath Pai @ 2016-09-27  7:42 UTC (permalink / raw)
  To: pablo
  Cc: kaber, kadlec, johunt, netfilter-devel, coreteam, netdev,
	pai.vishwain, zlpnobody

Fix link error in 32bit arch because of 64bit division

Division of 64bit integers will cause linker error undefined reference
to `__udivdi3'. Fix this by replacing divisions with div64_64

Signed-off-by: Vishwanath Pai <vpai@akamai.com>

---
 net/netfilter/xt_hashlimit.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/net/netfilter/xt_hashlimit.c b/net/netfilter/xt_hashlimit.c
index 44a095e..7fc694e 100644
--- a/net/netfilter/xt_hashlimit.c
+++ b/net/netfilter/xt_hashlimit.c
@@ -465,19 +465,20 @@ static u64 user2credits(u64 user, int revision)
 {
 	if (revision == 1) {
 		/* If multiplying would overflow... */
-		if (user > 0xFFFFFFFF / (HZ*CREDITS_PER_JIFFY_v1))
+		if (user > div64_u64(0xFFFFFFFF, (HZ*CREDITS_PER_JIFFY_v1)))
 			/* Divide first. */
-			return (user / XT_HASHLIMIT_SCALE) *\
+			return div64_u64(user, XT_HASHLIMIT_SCALE) *\
 						HZ * CREDITS_PER_JIFFY_v1;
 
-		return (user * HZ * CREDITS_PER_JIFFY_v1) \
-						/ XT_HASHLIMIT_SCALE;
+		return div64_u64((user * HZ * CREDITS_PER_JIFFY_v1),
+				  XT_HASHLIMIT_SCALE);
 	} else {
-		if (user > 0xFFFFFFFFFFFFFFFF / (HZ*CREDITS_PER_JIFFY))
-			return (user / XT_HASHLIMIT_SCALE_v2) *\
+		if (user > div64_u64(0xFFFFFFFFFFFFFFFF, (HZ*CREDITS_PER_JIFFY)))
+			return div64_u64(user, XT_HASHLIMIT_SCALE_v2) *\
 						HZ * CREDITS_PER_JIFFY;
 
-		return (user * HZ * CREDITS_PER_JIFFY) / XT_HASHLIMIT_SCALE_v2;
+		return div64_u64((user * HZ * CREDITS_PER_JIFFY),
+				 XT_HASHLIMIT_SCALE_v2);
 	}
 }
 
-- 
1.9.1


^ permalink raw reply related

* Re: [PATCH net-next 2/3] net: mpls: Fixups for GSO
From: Jiri Benc @ 2016-09-27  7:45 UTC (permalink / raw)
  To: David Ahern
  Cc: pravin shelar, Simon Horman, Pravin B Shelar,
	Linux Kernel Network Developers, David S. Miller, buytenh,
	Eric W. Biederman, rshearma, tom, Thomas Graf, olivier.dugeon,
	Alexander Duyck, roopa
In-Reply-To: <363ba109-1951-1018-060f-25df33c717e0@cumulusnetworks.com>

On Mon, 26 Sep 2016 20:04:06 -0600, David Ahern wrote:
> you know this code better than me, but key_extract pulls the eth
> header and then sets network header. If MPLS labels are present then
> it is the labels that the network_header now points to. How did come
> to the conclusion it is after the labels?

Look ~100 lines below that, to "if (eth_p_mpls(key->eth.type))".
There's a while loop advancing network header.

 Jiri

^ permalink raw reply

* Re: [PATCH net-next] net/sched: pkt_cls: change tc actions order to be as the user sets
From: Hadar Hen Zion @ 2016-09-27  7:46 UTC (permalink / raw)
  To: Cong Wang
  Cc: Jamal Hadi Salim, Hadar Hen Zion, David S. Miller,
	Linux Kernel Network Developers, Or Gerlitz
In-Reply-To: <CAM_iQpUxCU3S3sryGxnWBQKs6bOeVP49JotdFRur69-VV4+HzA@mail.gmail.com>

On Mon, Sep 26, 2016 at 11:34 PM, Cong Wang <xiyou.wangcong@gmail.com> wrote:
> On Sun, Sep 25, 2016 at 11:02 PM, Hadar Hen Zion
> <hadarh@dev.mellanox.co.il> wrote:
>> On Mon, Sep 26, 2016 at 7:31 AM, Cong Wang <xiyou.wangcong@gmail.com> wrote:
>>> On Sun, Sep 25, 2016 at 7:39 AM, Jamal Hadi Salim <jhs@mojatatu.com> wrote:
>>>> On 16-09-25 10:08 AM, Hadar Hen Zion wrote:
>>>>>
>>>>> Currently the created tc actions list is reversed against the order
>>>>> set by the user.
>>>>> Change the actions list order to be the same as was set by the user.
>>>>>
>>>>
>>>>
>>>> Did something break? It seems to matter most for dumping. But even that
>>>> didnt breaking. Looking at the latest net tree, i tried:
>>>>
>>>
>>> The reason is we use action->order as an nested attribute, so
>>> the order in the list doesn't matter, only action->order itself matters.
>>
>> The order in the list matters for offload drivers who use the
>> "tcf_exts_to_list" function and action->order parameter isn't usable
>> for them.
>> Why not keeping the actions in the same order as the user? isn't it
>> more elegant?
>
> I don't object this patch since it affects offloading, I just explained
> why it doesn't affect dumping.
>
> Please add this to your changelog, to make it obvious.

Sure, I'll add it.

Hadar

>
> Thanks!

^ permalink raw reply

* Re: [PATCH v3] net: ip, diag -- Add diag interface for raw sockets
From: Cyrill Gorcunov @ 2016-09-27  7:48 UTC (permalink / raw)
  To: David Ahern
  Cc: Eric Dumazet, netdev, linux-kernel, David Miller, kuznet, jmorris,
	yoshfuji, kaber, avagin, stephen
In-Reply-To: <fd77837c-433c-5010-7d07-68b0bf3e3d22@cumulusnetworks.com>

On Mon, Sep 26, 2016 at 07:54:37PM -0600, David Ahern wrote:
> On 9/26/16 4:38 PM, Cyrill Gorcunov wrote:
> > Something like
> > 
> > Index: linux-ml.git/include/uapi/linux/inet_diag.h
> > ===================================================================
> > --- linux-ml.git.orig/include/uapi/linux/inet_diag.h    2016-09-11 20:56:18.191584145 +0300
> > +++ linux-ml.git/include/uapi/linux/inet_diag.h 2016-09-27 01:34:08.413172394 +0300
> > @@ -38,7 +38,7 @@ struct inet_diag_req_v2 {
> >         __u8    sdiag_family;
> >         __u8    sdiag_protocol;
> >         __u8    idiag_ext;
> > -       __u8    pad;
> > +       __u8    sdiag_raw_protocol;     /* SOCK_RAW only, @pad for others */
> 
> Seems like that should be a union to keep the API.

Is anonymous union (which is not part of c99) are acceptable in uapi?
Initially I declared it as union but then scratched my head if this
would be acceptable.

> 
> 
> >         __u32   idiag_states;
> >         struct inet_diag_sockid id;
> >  };
> > 
> > and in raw-diag module we will use @sdiag_raw_protocol instead of
> > @sdiag_protocol field. Didn't cover ss tool source code yet but
> > I think the idea is seen. Still not sure if start using @pad here
> > is a good idea (it's uapi), maybe beter to ask nla attribute which would
> > come right afterh the inet_diag_req_v2 message?
> > 
> 
> seems reasonable to me since 2 protocols need to be sent to the kernel.
> 
> Alternatively, sdiag_protocol could be the actual protocol and the pad union be a flag field
> with say bit 0 = INET_DIAG_FLAG_SOCK_RAW. Allows other overrides in the future if needed.

The @sdiag_protocol used for matching in diag module handler, so no, I think
we should not change this semantics. I would stick with @pad usage and if
anonymous unions are acceptable this would be just great.

	Cyrill

^ permalink raw reply

* Re: [PATCH 1/2] bpf samples: fix compiler errors with sockex2 and sockex3
From: David Miller @ 2016-09-27  7:49 UTC (permalink / raw)
  To: naveen.n.rao; +Cc: linux-kernel, linuxppc-dev, netdev, mpe, ast, daniel, ananth
In-Reply-To: <7b29fba6073924dc4c9e7d639eefb0b15a98660d.1474661952.git.naveen.n.rao@linux.vnet.ibm.com>

From: "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com>
Date: Sat, 24 Sep 2016 02:10:04 +0530

> These samples fail to compile as 'struct flow_keys' conflicts with
> definition in net/flow_dissector.h. Fix the same by renaming the
> structure used in the sample.
> 
> Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>

Applied to net-next.

^ permalink raw reply

* Re: [PATCH 2/2] bpf samples: update tracex5 sample to use __seccomp_filter
From: David Miller @ 2016-09-27  7:49 UTC (permalink / raw)
  To: naveen.n.rao; +Cc: linux-kernel, linuxppc-dev, netdev, mpe, ast, daniel, ananth
In-Reply-To: <8e5ab9853508240e901260b1af36243c4d744083.1474661952.git.naveen.n.rao@linux.vnet.ibm.com>

From: "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com>
Date: Sat, 24 Sep 2016 02:10:05 +0530

> seccomp_phase1() does not exist anymore. Instead, update sample to use
> __seccomp_filter(). While at it, set max locked memory to unlimited.
> 
> Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>

Also applied to net-next, thanks.

^ permalink raw reply

* Re: [PATCH v3] bpf: Set register type according to is_valid_access()
From: David Miller @ 2016-09-27  7:52 UTC (permalink / raw)
  To: mic; +Cc: linux-kernel, ast, luto, daniel, keescook, sargun, tj, netdev
In-Reply-To: <20160924180150.23620-1-mic@digikod.net>

From: Mickaël Salaün <mic@digikod.net>
Date: Sat, 24 Sep 2016 20:01:50 +0200

> This prevent future potential pointer leaks when an unprivileged eBPF
> program will read a pointer value from its context. Even if
> is_valid_access() returns a pointer type, the eBPF verifier replace it
> with UNKNOWN_VALUE. The register value that contains a kernel address is
> then allowed to leak. Moreover, this fix allows unprivileged eBPF
> programs to use functions with (legitimate) pointer arguments.
> 
> Not an issue currently since reg_type is only set for PTR_TO_PACKET or
> PTR_TO_PACKET_END in XDP and TC programs that can only be loaded as
> privileged. For now, the only unprivileged eBPF program allowed is for
> socket filtering and all the types from its context are UNKNOWN_VALUE.
> However, this fix is important for future unprivileged eBPF programs
> which could use pointers in their context.
> 
> Signed-off-by: Mickaël Salaün <mic@digikod.net>

Applied to net-next, thanks.

^ permalink raw reply

* Re: [PATCH v2] fs/select: add vmalloc fallback for select(2)
From: Vlastimil Babka @ 2016-09-27  8:06 UTC (permalink / raw)
  To: Andrew Morton
  Cc: Alexander Viro, linux-fsdevel, linux-kernel, linux-mm,
	Michal Hocko, netdev, Eric Dumazet
In-Reply-To: <20160926170105.517f74cd67ecdd5ef73e1865@linux-foundation.org>

On 09/27/2016 02:01 AM, Andrew Morton wrote:
> On Thu, 22 Sep 2016 18:43:59 +0200 Vlastimil Babka <vbabka@suse.cz> wrote:
>
>> The select(2) syscall performs a kmalloc(size, GFP_KERNEL) where size grows
>> with the number of fds passed. We had a customer report page allocation
>> failures of order-4 for this allocation. This is a costly order, so it might
>> easily fail, as the VM expects such allocation to have a lower-order fallback.
>>
>> Such trivial fallback is vmalloc(), as the memory doesn't have to be
>> physically contiguous. Also the allocation is temporary for the duration of the
>> syscall, so it's unlikely to stress vmalloc too much.
>>
>> Note that the poll(2) syscall seems to use a linked list of order-0 pages, so
>> it doesn't need this kind of fallback.
>>
>> ...
>>
>> --- a/fs/select.c
>> +++ b/fs/select.c
>> @@ -29,6 +29,7 @@
>>  #include <linux/sched/rt.h>
>>  #include <linux/freezer.h>
>>  #include <net/busy_poll.h>
>> +#include <linux/vmalloc.h>
>>
>>  #include <asm/uaccess.h>
>>
>> @@ -558,6 +559,7 @@ int core_sys_select(int n, fd_set __user *inp, fd_set __user *outp,
>>  	struct fdtable *fdt;
>>  	/* Allocate small arguments on the stack to save memory and be faster */
>>  	long stack_fds[SELECT_STACK_ALLOC/sizeof(long)];
>> +	unsigned long alloc_size;
>>
>>  	ret = -EINVAL;
>>  	if (n < 0)
>> @@ -580,8 +582,12 @@ int core_sys_select(int n, fd_set __user *inp, fd_set __user *outp,
>>  	bits = stack_fds;
>>  	if (size > sizeof(stack_fds) / 6) {
>>  		/* Not enough space in on-stack array; must use kmalloc */
>> +		alloc_size = 6 * size;
>
> Well.  `size' is `unsigned'.  The multiplication will be done as 32-bit
> so there was no point in making `alloc_size' unsigned long.

Uh, right. Thanks.

> So can we tighten up the types in this function?  size_t might make
> sense, but vmalloc() takes a ulong.

Let's do size_t then, as the conversion to ulong is safe.


--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

^ permalink raw reply

* Re: [PATCH net-next 4/4] net/sched: act_mirred: Implement ingress actions
From: Shmulik Ladkani @ 2016-09-27  8:07 UTC (permalink / raw)
  To: David Miller; +Cc: jhs, xiyou.wangcong, edumazet, netdev, shmulik.ladkani
In-Reply-To: <20160927.015606.437705429903770747.davem@davemloft.net>

Hi David,

On Tue, 27 Sep 2016 01:56:06 -0400 (EDT), davem@davemloft.net wrote:
> The discussion on this patch has ventured off into what to do about
> recursion.
> 
> But it unclear to me where this specific patch, and this series,
> stands right now.  Someone please clear this up for me.

Status:
 - Series adds "ingress redirect/mirror" support
 - Positive feedback for the feature
 - So far no comments regarding code itself
 - Questions raised regarding "recursion handling"
   Expressed that existing mirred code (i.e egress redirect) is *already*
   loop-unsafe (and also, some non-tc netdev constructs, as exampled by
   others).
   Discussion then wandered to "recursion handling".

Regards,
Shmulik	

^ permalink raw reply

* [PATCH net-next V2] net/sched: pkt_cls: change tc actions order to be as the user sets
From: Hadar Hen Zion @ 2016-09-27  8:09 UTC (permalink / raw)
  To: David S. Miller
  Cc: netdev, Jamal Hadi Salim, Cong Wang, Or Gerlitz, Hadar Hen Zion

Currently the created tc actions list is reversed against the order
set by the user.
Change the actions list order to be the same as was set by the user.

This patch doesn't affect dump actions behavior.
For dumping, action->order parameter is used so the list order doesn't
matter.

Signed-off-by: Hadar Hen Zion <hadarh@mellanox.com>
Acked-by: Jamal Hadi Salim <jhs@mojatatu.com>
---
 include/net/pkt_cls.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/net/pkt_cls.h b/include/net/pkt_cls.h
index 5ccaa4b..767b03a 100644
--- a/include/net/pkt_cls.h
+++ b/include/net/pkt_cls.h
@@ -123,7 +123,7 @@ static inline void tcf_exts_to_list(const struct tcf_exts *exts,
 	for (i = 0; i < exts->nr_actions; i++) {
 		struct tc_action *a = exts->actions[i];
 
-		list_add(&a->list, actions);
+		list_add_tail(&a->list, actions);
 	}
 #endif
 }
-- 
1.8.3.1

^ permalink raw reply related

* Re: [PATCH v2] fs/select: add vmalloc fallback for select(2)
From: Vlastimil Babka @ 2016-09-27  8:13 UTC (permalink / raw)
  To: Eric Dumazet, Andrew Morton
  Cc: Alexander Viro, linux-fsdevel, linux-kernel, linux-mm,
	Michal Hocko, netdev
In-Reply-To: <1474940324.28155.44.camel@edumazet-glaptop3.roam.corp.google.com>

On 09/27/2016 03:38 AM, Eric Dumazet wrote:
> On Mon, 2016-09-26 at 17:01 -0700, Andrew Morton wrote:
>
>> I don't share Eric's concerns about performance here.  If the vmalloc()
>> is called, we're about to write to that quite large amount of memory
>> which we just allocated, and the vmalloc() overhead will be relatively
>> low.
>
> I did not care of the performance of this particular select() system
> call really, but other cpus because of more TLB invalidations.

There are many other ways to cause those, AFAIK. The reclaim/compaction
for order-3 allocation has its own impact on system, including TLB flushes.
Or a flood of mmap(MAP_POPULATE) and madvise(MADV_DONTNEED) calls...
This vmalloc() would however require raising RLIMIT_NOFILE above the defaults.

> At least CONFIG_DEBUG_PAGEALLOC=y builds should be impacted, but maybe
> we do not care.

I doubt anyone runs that in production, especially if performance is of concern.

^ permalink raw reply

* [PATCH net-next] net/sched: cls_flower: Use a proper mask value for enc key id parameter
From: Hadar Hen Zion @ 2016-09-27  8:21 UTC (permalink / raw)
  To: David S. Miller
  Cc: netdev, Amir Vadai, Jiri Pirko, Or Gerlitz, Hadar Hen Zion

The current code use the encapsulation key id value as the mask of that
parameter which is wrong. Fix that by using a full mask.

Fixes: bc3103f1ed40 ('net/sched: cls_flower: Classify packet in ip tunnels')
Signed-off-by: Hadar Hen Zion <hadarh@mellanox.com>
---
 net/sched/cls_flower.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/sched/cls_flower.c b/net/sched/cls_flower.c
index 2af09c8..f6f40fb 100644
--- a/net/sched/cls_flower.c
+++ b/net/sched/cls_flower.c
@@ -481,7 +481,7 @@ static int fl_set_key(struct net *net, struct nlattr **tb,
 	}
 
 	fl_set_key_val(tb, &key->enc_key_id.keyid, TCA_FLOWER_KEY_ENC_KEY_ID,
-		       &mask->enc_key_id.keyid, TCA_FLOWER_KEY_ENC_KEY_ID,
+		       &mask->enc_key_id.keyid, TCA_FLOWER_UNSPEC,
 		       sizeof(key->enc_key_id.keyid));
 
 	return 0;
@@ -919,7 +919,7 @@ static int fl_dump(struct net *net, struct tcf_proto *tp, unsigned long fh,
 		goto nla_put_failure;
 
 	if (fl_dump_key_val(skb, &key->enc_key_id, TCA_FLOWER_KEY_ENC_KEY_ID,
-			    &mask->enc_key_id, TCA_FLOWER_KEY_ENC_KEY_ID,
+			    &mask->enc_key_id, TCA_FLOWER_UNSPEC,
 			    sizeof(key->enc_key_id)))
 		goto nla_put_failure;
 
-- 
1.8.3.1

^ permalink raw reply related

* Re: [PATCH] fs/select: add vmalloc fallback for select(2)
From: Vlastimil Babka @ 2016-09-27  8:44 UTC (permalink / raw)
  To: Jason Baron, Nicholas Piggin, Hillf Danton
  Cc: 'Alexander Viro', linux-fsdevel, linux-kernel, linux-mm,
	'Michal Hocko', netdev, Eric Dumazet
In-Reply-To: <57E55CBB.5060309@akamai.com>

On 09/23/2016 06:47 PM, Jason Baron wrote:
> Hi,
>
> On 09/23/2016 03:24 AM, Nicholas Piggin wrote:
>> On Fri, 23 Sep 2016 14:42:53 +0800
>> "Hillf Danton" <hillf.zj@alibaba-inc.com> wrote:
>>
>>>>
>>>> The select(2) syscall performs a kmalloc(size, GFP_KERNEL) where size grows
>>>> with the number of fds passed. We had a customer report page allocation
>>>> failures of order-4 for this allocation. This is a costly order, so it might
>>>> easily fail, as the VM expects such allocation to have a lower-order fallback.
>>>>
>>>> Such trivial fallback is vmalloc(), as the memory doesn't have to be
>>>> physically contiguous. Also the allocation is temporary for the duration of the
>>>> syscall, so it's unlikely to stress vmalloc too much.
>>>>
>>>> Note that the poll(2) syscall seems to use a linked list of order-0 pages, so
>>>> it doesn't need this kind of fallback.
>>
>> How about something like this? (untested)

This pushes the limit further, but might just delay the problem. Could be an 
optimization on top if there's enough interest, though.

[...]

>> +
>> +		if (!(fds.in && fds.out && fds.ex &&
>> +				fds.res_in && fds.res_out && fds.res_ex))
>> +			goto out;
>> +	} else {
>> +		if (nr_bytes > sizeof(stack_fds)) {
>> +			/* Not enough space in on-stack array */
>> +			if (nr_bytes > PAGE_SIZE * 2)
>
> The 'if' looks extraneous?
>
> Also, I wonder if we can just avoid some allocations altogether by
> checking by if the user fd_set pointers are NULL? That can avoid failures :)

That would be a more major rewrite, as the core algorithm doesn't expect NULLs.

> Thanks,
>
> -Jason
>

^ permalink raw reply

* [PATCH v3] fs/select: add vmalloc fallback for select(2)
From: Vlastimil Babka @ 2016-09-27  8:45 UTC (permalink / raw)
  To: Alexander Viro, Andrew Morton
  Cc: linux-fsdevel, linux-kernel, linux-mm, Michal Hocko, netdev,
	Eric Dumazet, David Laight, Hillf Danton, Nicholas Piggin,
	Jason Baron, Vlastimil Babka
In-Reply-To: <20160922164359.9035-1-vbabka@suse.cz>

The select(2) syscall performs a kmalloc(size, GFP_KERNEL) where size grows
with the number of fds passed. We had a customer report page allocation
failures of order-4 for this allocation. This is a costly order, so it might
easily fail, as the VM expects such allocation to have a lower-order fallback.

Such trivial fallback is vmalloc(), as the memory doesn't have to be physically
contiguous and the allocation is temporary for the duration of the syscall
only. There were some concerns, whether this would have negative impact on the
system by exposing vmalloc() to userspace. Although an excessive use of vmalloc
can cause some system wide performance issues - TLB flushes etc. - a large
order allocation is not for free either and an excessive reclaim/compaction can
have a similar effect. Also note that the size is effectively limited by
RLIMIT_NOFILE which defaults to 1024 on the systems I checked. That means the
bitmaps will fit well within single page and thus the vmalloc() fallback could
be only excercised for processes where root allows a higher limit.

Note that the poll(2) syscall seems to use a linked list of order-0 pages, so
it doesn't need this kind of fallback.

[eric.dumazet@gmail.com: fix failure path logic]
[akpm@linux-foundation.org: use proper type for size]
Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
---
 fs/select.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/fs/select.c b/fs/select.c
index 8ed9da50896a..3d4f85defeab 100644
--- a/fs/select.c
+++ b/fs/select.c
@@ -29,6 +29,7 @@
 #include <linux/sched/rt.h>
 #include <linux/freezer.h>
 #include <net/busy_poll.h>
+#include <linux/vmalloc.h>
 
 #include <asm/uaccess.h>
 
@@ -554,7 +555,7 @@ int core_sys_select(int n, fd_set __user *inp, fd_set __user *outp,
 	fd_set_bits fds;
 	void *bits;
 	int ret, max_fds;
-	unsigned int size;
+	size_t size, alloc_size;
 	struct fdtable *fdt;
 	/* Allocate small arguments on the stack to save memory and be faster */
 	long stack_fds[SELECT_STACK_ALLOC/sizeof(long)];
@@ -581,7 +582,14 @@ int core_sys_select(int n, fd_set __user *inp, fd_set __user *outp,
 	if (size > sizeof(stack_fds) / 6) {
 		/* Not enough space in on-stack array; must use kmalloc */
 		ret = -ENOMEM;
-		bits = kmalloc(6 * size, GFP_KERNEL);
+		if (size > (SIZE_MAX / 6))
+			goto out_nofds;
+
+		alloc_size = 6 * size;
+		bits = kmalloc(alloc_size, GFP_KERNEL|__GFP_NOWARN);
+		if (!bits && alloc_size > PAGE_SIZE)
+			bits = vmalloc(alloc_size);
+
 		if (!bits)
 			goto out_nofds;
 	}
@@ -618,7 +626,7 @@ int core_sys_select(int n, fd_set __user *inp, fd_set __user *outp,
 
 out:
 	if (bits != stack_fds)
-		kfree(bits);
+		kvfree(bits);
 out_nofds:
 	return ret;
 }
-- 
2.10.0

^ permalink raw reply related

* Re: ISDN-Gigaset: Release memory in gigaset_initcs() after an allocation failure
From: Paul Bolle @ 2016-09-27  8:48 UTC (permalink / raw)
  To: SF Markus Elfring
  Cc: gigaset307x-common, netdev, Karsten Keil, LKML, kernel-janitors,
	Julia Lawall
In-Reply-To: <0d02085e-842c-4be9-f706-c43b38ce9095@users.sourceforge.net>

On Tue, 2016-09-27 at 07:20 +0200, SF Markus Elfring wrote:
> Will it matter here if the function "kfree" will be called for the
> data structure members "bcs" and "inbuf" after a later function call
> failed within the implementation of "gigaset_initcs"?

My translation of this question is: could you please hold my hand while
I read the code of a driver I do not use - a driver for hardware that I
don't even have, and therefor cannot really test - after I submitted a
patch that appears to be broken?

My answer to that question is: no, sorry, I won't do that.


Paul Bolle

^ permalink raw reply

* [PATCH RFC 1/3] net: Add dev_set_env_hdr_len to accept envelope frames
From: Toshiaki Makita @ 2016-09-27  8:55 UTC (permalink / raw)
  To: netdev, Patrick McHardy, Stephen Hemminger, Vlad Yasevich,
	Jeff Kirsher
  Cc: Toshiaki Makita
In-Reply-To: <1474966541-4420-1-git-send-email-makita.toshiaki@lab.ntt.co.jp>

Currently most NICs support Q-tagged frames[1], i.e. 1522 bytes frames
to handle 4 bytes VLAN header. But some encapsulation protocols like
802.1ad requires them to handle larger frames.
This change introduces dev_set_env_hdr_len() and corresponding drivers'
operation .ndo_set_env_hdr_len(), which notifies drivers of needed
encapsulation header length. This enables devices to accept longer
frames with encapsulation headers, i.e. envelope frames[2], without
expanding MTU size for non-encapsulated frames.

Note 1:
Envelope frames are not jumbo frames. See IEEE 802.3as[3] for detail.
IEEE 802.3-2012 3.2.7 says:
          The envelope frame is intended to allow inclusion of additional
	  prefixes and suffixes required by higher layer encapusulation
	  protocols such as those defined by the IEEE 802.1 working
	  group (such as Provider Bridges and MAC Security), ITU-T or
	  IETF (such as MPLS). The original MAC Client Data field
	  maximum remains 1500 octets while the encapsulation protocols
	  may add up to an additional 482 octets. Use of these extra
	  octets for other purposes is not recommended, and may result
	  in MAC frames being dropped or corrupted as they may violate
	  maximum MAC frame size restrictions if encapsulation protocols
	  are required to operate on them.

Note 2:
Envelope frames in IEEE 802.3 defines the max size of envelope frames
as 2000 bytes. This change is more flexible than 802.3 in terms of max
allowed frame length.

[1] IEEE 802.3-2012, 1.4.334.
[2] IEEE 802.3-2012, 1.4.184.
[3] http://www.ieee802.org/3/as/public/0607/802.3as_overview.pdf

Signed-off-by: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>
---
 include/linux/netdevice.h | 21 +++++++++++++++++++++
 net/core/dev.c            | 32 ++++++++++++++++++++++++++++++++
 2 files changed, 53 insertions(+)

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 136ae6bb..a0ac76a 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -1132,6 +1132,10 @@ struct netdev_xdp {
  * int (*ndo_xdp)(struct net_device *dev, struct netdev_xdp *xdp);
  *	This function is used to set or query state related to XDP on the
  *	netdevice. See definition of enum xdp_netdev_command for details.
+ * int (*ndo_set_env_hdr_len)(struct net_device *dev, int hdr_len);
+ *	This function is used to set the maximum header size of envelope
+ *	frames. The device must accept the size of MTU + envelope header
+ *	size on packet reception.
  *
  */
 struct net_device_ops {
@@ -1323,6 +1327,8 @@ struct net_device_ops {
 						       int needed_headroom);
 	int			(*ndo_xdp)(struct net_device *dev,
 					   struct netdev_xdp *xdp);
+	int			(*ndo_set_env_hdr_len)(struct net_device *dev,
+						       int hdr_len);
 };
 
 /**
@@ -1506,6 +1512,7 @@ enum netdev_priv_flags {
  *	@if_port:	Selectable AUI, TP, ...
  *	@dma:		DMA channel
  *	@mtu:		Interface MTU value
+ *	@env_hdr_len:	Additional encapsulation header length to MTU
  *	@type:		Interface hardware type
  *	@hard_header_len: Maximum hardware header length.
  *
@@ -1726,6 +1733,7 @@ struct net_device {
 	unsigned char		dma;
 
 	unsigned int		mtu;
+	unsigned int		env_hdr_len;
 	unsigned short		type;
 	unsigned short		hard_header_len;
 
@@ -3300,6 +3308,7 @@ int dev_change_name(struct net_device *, const char *);
 int dev_set_alias(struct net_device *, const char *, size_t);
 int dev_change_net_namespace(struct net_device *, struct net *, const char *);
 int dev_set_mtu(struct net_device *, int);
+int dev_set_env_hdr_len(struct net_device *, int);
 void dev_set_group(struct net_device *, int);
 int dev_set_mac_address(struct net_device *, struct sockaddr *);
 int dev_change_carrier(struct net_device *, bool new_carrier);
@@ -4233,6 +4242,18 @@ static inline bool netif_reduces_vlan_mtu(struct net_device *dev)
 	return dev->priv_flags & IFF_MACSEC;
 }
 
+/* return envelope header length */
+static inline int netif_get_env_hdr_len(struct net_device *dev)
+{
+	if (dev->netdev_ops->ndo_set_env_hdr_len)
+		return dev->env_hdr_len;
+
+	if (netif_reduces_vlan_mtu(dev))
+		return 0;
+
+	return 4; /* VLAN_HLEN */
+}
+
 extern struct pernet_operations __net_initdata loopback_net_ops;
 
 /* Logging, debugging and troubleshooting/diagnostic helpers. */
diff --git a/net/core/dev.c b/net/core/dev.c
index c0c291f..df75aaa 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -6524,6 +6524,38 @@ int dev_set_mtu(struct net_device *dev, int new_mtu)
 EXPORT_SYMBOL(dev_set_mtu);
 
 /**
+ *	dev_set_env_hdr_len - Set max envelope header length
+ *	@dev: device
+ *	@new_len: new length
+ */
+int dev_set_env_hdr_len(struct net_device *dev, int new_len)
+{
+	const struct net_device_ops *ops = dev->netdev_ops;
+	int err;
+
+	if (!ops->ndo_set_env_hdr_len)
+		return -EOPNOTSUPP;
+
+	if (new_len < 0)
+		return -EINVAL;
+
+	if (!netif_device_present(dev))
+		return -ENODEV;
+
+	if (new_len == dev->env_hdr_len)
+		return 0;
+
+	err = ops->ndo_set_env_hdr_len(dev, new_len);
+	if (err)
+		return err;
+
+	dev->env_hdr_len = new_len;
+
+	return 0;
+}
+EXPORT_SYMBOL(dev_set_env_hdr_len);
+
+/**
  *	dev_set_group - Change group this device belongs to
  *	@dev: device
  *	@new_group: group this device should belong to
-- 
1.8.3.1

^ permalink raw reply related

* [PATCH RFC iproute2] iplink: Support envhdrlen
From: Toshiaki Makita @ 2016-09-27  8:55 UTC (permalink / raw)
  To: netdev, Patrick McHardy, Stephen Hemminger, Vlad Yasevich,
	Jeff Kirsher
  Cc: Toshiaki Makita
In-Reply-To: <1474966541-4420-1-git-send-email-makita.toshiaki@lab.ntt.co.jp>

This adds support for envhdrlen.

Example:
 # ip link set eno1 envhdrlen 8
 # ip link show eno1
 2: eno1: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 envhdrlen 8 qdisc fq_codel state UP mode DEFAULT group default qlen 1000
     link/ether 44:37:e6:6c:69:a4 brd ff:ff:ff:ff:ff:ff

Note:
As an RFC, this includes update for kernel headers.

Signed-off-by: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>
---
 include/linux/if_link.h |  1 +
 ip/ipaddress.c          |  2 ++
 ip/iplink.c             | 10 ++++++++++
 3 files changed, 13 insertions(+)

diff --git a/include/linux/if_link.h b/include/linux/if_link.h
index b9299e3..46ef8cc 100644
--- a/include/linux/if_link.h
+++ b/include/linux/if_link.h
@@ -157,6 +157,7 @@ enum {
 	IFLA_GSO_MAX_SIZE,
 	IFLA_PAD,
 	IFLA_XDP,
+	IFLA_ENV_HDR_LEN,
 	__IFLA_MAX
 };
 
diff --git a/ip/ipaddress.c b/ip/ipaddress.c
index 76bd7b3..92a472d 100644
--- a/ip/ipaddress.c
+++ b/ip/ipaddress.c
@@ -820,6 +820,8 @@ int print_linkinfo(const struct sockaddr_nl *who,
 
 	if (tb[IFLA_MTU])
 		fprintf(fp, "mtu %u ", *(int *)RTA_DATA(tb[IFLA_MTU]));
+	if (tb[IFLA_ENV_HDR_LEN])
+		fprintf(fp, "envhdrlen %u ", *(int *)RTA_DATA(tb[IFLA_ENV_HDR_LEN]));
 	if (tb[IFLA_QDISC])
 		fprintf(fp, "qdisc %s ", rta_getattr_str(tb[IFLA_QDISC]));
 	if (tb[IFLA_MASTER]) {
diff --git a/ip/iplink.c b/ip/iplink.c
index 6b1db18..4dcb9ac 100644
--- a/ip/iplink.c
+++ b/ip/iplink.c
@@ -50,6 +50,7 @@ void iplink_usage(void)
 		fprintf(stderr, "                   [ address LLADDR ]\n");
 		fprintf(stderr, "                   [ broadcast LLADDR ]\n");
 		fprintf(stderr, "                   [ mtu MTU ] [index IDX ]\n");
+		fprintf(stderr, "                   [ envhdrlen ENVHDRLEN ]\n");
 		fprintf(stderr, "                   [ numtxqueues QUEUE_COUNT ]\n");
 		fprintf(stderr, "                   [ numrxqueues QUEUE_COUNT ]\n");
 		fprintf(stderr, "                   type TYPE [ ARGS ]\n");
@@ -489,6 +490,7 @@ int iplink_parse(int argc, char **argv, struct iplink_req *req,
 	char abuf[32];
 	int qlen = -1;
 	int mtu = -1;
+	int envhdrlen = -1;
 	int netns = -1;
 	int vf = -1;
 	int numtxqueues = -1;
@@ -547,6 +549,14 @@ int iplink_parse(int argc, char **argv, struct iplink_req *req,
 			if (get_integer(&mtu, *argv, 0))
 				invarg("Invalid \"mtu\" value\n", *argv);
 			addattr_l(&req->n, sizeof(*req), IFLA_MTU, &mtu, 4);
+		} else if (strcmp(*argv, "envhdrlen") == 0) {
+			NEXT_ARG();
+			if (envhdrlen != -1)
+				duparg("envhdrlen", *argv);
+			if (get_integer(&envhdrlen, *argv, 0))
+				invarg("Invalid \"envhdrlen\" value\n", *argv);
+			addattr_l(&req->n, sizeof(*req), IFLA_ENV_HDR_LEN,
+				  &envhdrlen, 4);
 		} else if (strcmp(*argv, "netns") == 0) {
 			NEXT_ARG();
 			if (netns != -1)
-- 
2.5.5

^ permalink raw reply related

* [PATCH RFC 2/3] net: Support IFLA_ENV_HDR_LEN to configure max envelope header length
From: Toshiaki Makita @ 2016-09-27  8:55 UTC (permalink / raw)
  To: netdev, Patrick McHardy, Stephen Hemminger, Vlad Yasevich,
	Jeff Kirsher
  Cc: Toshiaki Makita
In-Reply-To: <1474966541-4420-1-git-send-email-makita.toshiaki@lab.ntt.co.jp>

With this change, admin can configure env_hdr_len.

Signed-off-by: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>
---
 include/uapi/linux/if_link.h |  1 +
 net/core/rtnetlink.c         | 16 ++++++++++++++--
 2 files changed, 15 insertions(+), 2 deletions(-)

diff --git a/include/uapi/linux/if_link.h b/include/uapi/linux/if_link.h
index b4fba66..9545ea4 100644
--- a/include/uapi/linux/if_link.h
+++ b/include/uapi/linux/if_link.h
@@ -157,6 +157,7 @@ enum {
 	IFLA_GSO_MAX_SIZE,
 	IFLA_PAD,
 	IFLA_XDP,
+	IFLA_ENV_HDR_LEN,
 	__IFLA_MAX
 };
 
diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index 3ac8946..9233709 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -943,7 +943,8 @@ static noinline size_t if_nlmsg_size(const struct net_device *dev,
 	       + nla_total_size(MAX_PHYS_ITEM_ID_LEN) /* IFLA_PHYS_SWITCH_ID */
 	       + nla_total_size(IFNAMSIZ) /* IFLA_PHYS_PORT_NAME */
 	       + rtnl_xdp_size(dev) /* IFLA_XDP */
-	       + nla_total_size(1); /* IFLA_PROTO_DOWN */
+	       + nla_total_size(1) /* IFLA_PROTO_DOWN */
+	       + nla_total_size(4); /* IFLA_ENV_HDR_LEN */
 
 }
 
@@ -1321,7 +1322,8 @@ static int rtnl_fill_ifinfo(struct sk_buff *skb, struct net_device *dev,
 	     nla_put_string(skb, IFLA_IFALIAS, dev->ifalias)) ||
 	    nla_put_u32(skb, IFLA_CARRIER_CHANGES,
 			atomic_read(&dev->carrier_changes)) ||
-	    nla_put_u8(skb, IFLA_PROTO_DOWN, dev->proto_down))
+	    nla_put_u8(skb, IFLA_PROTO_DOWN, dev->proto_down) ||
+	    nla_put_u32(skb, IFLA_ENV_HDR_LEN, netif_get_env_hdr_len(dev)))
 		goto nla_put_failure;
 
 	if (rtnl_fill_link_ifmap(skb, dev))
@@ -1458,6 +1460,7 @@ static const struct nla_policy ifla_policy[IFLA_MAX+1] = {
 	[IFLA_LINK_NETNSID]	= { .type = NLA_S32 },
 	[IFLA_PROTO_DOWN]	= { .type = NLA_U8 },
 	[IFLA_XDP]		= { .type = NLA_NESTED },
+	[IFLA_ENV_HDR_LEN]	= { .type = NLA_U32 },
 };
 
 static const struct nla_policy ifla_info_policy[IFLA_INFO_MAX+1] = {
@@ -2174,6 +2177,13 @@ static int do_setlink(const struct sk_buff *skb,
 		}
 	}
 
+	if (tb[IFLA_ENV_HDR_LEN]) {
+		err = dev_set_env_hdr_len(dev, nla_get_u32(tb[IFLA_ENV_HDR_LEN]));
+		if (err < 0)
+			goto errout;
+		status |= DO_SETLINK_MODIFIED;
+	}
+
 errout:
 	if (status & DO_SETLINK_MODIFIED) {
 		if (status & DO_SETLINK_NOTIFY)
@@ -2378,6 +2388,8 @@ struct net_device *rtnl_create_link(struct net *net,
 		dev->link_mode = nla_get_u8(tb[IFLA_LINKMODE]);
 	if (tb[IFLA_GROUP])
 		dev_set_group(dev, nla_get_u32(tb[IFLA_GROUP]));
+	if (tb[IFLA_ENV_HDR_LEN])
+		dev->env_hdr_len = nla_get_u32(tb[IFLA_ENV_HDR_LEN]);
 
 	return dev;
 
-- 
1.8.3.1

^ permalink raw reply related

* [PATCH RFC 3/3] e1000e: Add ndo_set_env_hdr_len
From: Toshiaki Makita @ 2016-09-27  8:55 UTC (permalink / raw)
  To: netdev, Patrick McHardy, Stephen Hemminger, Vlad Yasevich,
	Jeff Kirsher
  Cc: Toshiaki Makita
In-Reply-To: <1474966541-4420-1-git-send-email-makita.toshiaki@lab.ntt.co.jp>

e1000e supports generic 1522-sized frames by default, so set the default
env_hdr_len to 4, and replace dev->mtu with dev->mtu + env_hdr_len.
Note that e1000e has adapter->max_frame_size that includes mtu +
env_hdr_len, so I use it where mtu was used to validate frame length.

Signed-off-by: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>
---
 drivers/net/ethernet/intel/e1000e/netdev.c | 84 +++++++++++++++++++++---------
 1 file changed, 60 insertions(+), 24 deletions(-)

diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c b/drivers/net/ethernet/intel/e1000e/netdev.c
index 7017281..4dc9315 100644
--- a/drivers/net/ethernet/intel/e1000e/netdev.c
+++ b/drivers/net/ethernet/intel/e1000e/netdev.c
@@ -3033,7 +3033,7 @@ static void e1000_setup_rctl(struct e1000_adapter *adapter)
 	if (hw->mac.type >= e1000_pch2lan) {
 		s32 ret_val;
 
-		if (adapter->netdev->mtu > ETH_DATA_LEN)
+		if (adapter->max_frame_size > VLAN_ETH_FRAME_LEN + ETH_FCS_LEN)
 			ret_val = e1000_lv_jumbo_workaround_ich8lan(hw, true);
 		else
 			ret_val = e1000_lv_jumbo_workaround_ich8lan(hw, false);
@@ -3053,7 +3053,7 @@ static void e1000_setup_rctl(struct e1000_adapter *adapter)
 	rctl &= ~E1000_RCTL_SBP;
 
 	/* Enable Long Packet receive */
-	if (adapter->netdev->mtu <= ETH_DATA_LEN)
+	if (adapter->max_frame_size <= VLAN_ETH_FRAME_LEN + ETH_FCS_LEN)
 		rctl &= ~E1000_RCTL_LPE;
 	else
 		rctl |= E1000_RCTL_LPE;
@@ -3121,7 +3121,8 @@ static void e1000_setup_rctl(struct e1000_adapter *adapter)
 	 * a lot of memory, since we allocate 3 pages at all times
 	 * per packet.
 	 */
-	pages = PAGE_USE_COUNT(adapter->netdev->mtu);
+	pages = PAGE_USE_COUNT(adapter->netdev->mtu +
+			       adapter->netdev->env_hdr_len);
 	if ((pages <= 3) && (PAGE_SIZE <= 16384) && (rctl & E1000_RCTL_LPE))
 		adapter->rx_ps_pages = pages;
 	else
@@ -3191,7 +3192,8 @@ static void e1000_configure_rx(struct e1000_adapter *adapter)
 		    sizeof(union e1000_rx_desc_packet_split);
 		adapter->clean_rx = e1000_clean_rx_irq_ps;
 		adapter->alloc_rx_buf = e1000_alloc_rx_buffers_ps;
-	} else if (adapter->netdev->mtu > ETH_FRAME_LEN + ETH_FCS_LEN) {
+	} else if (adapter->netdev->mtu + adapter->netdev->env_hdr_len >
+		   ETH_FRAME_LEN + ETH_FCS_LEN) {
 		rdlen = rx_ring->count * sizeof(union e1000_rx_desc_extended);
 		adapter->clean_rx = e1000_clean_jumbo_rx_irq;
 		adapter->alloc_rx_buf = e1000_alloc_jumbo_rx_buffers;
@@ -3273,7 +3275,7 @@ static void e1000_configure_rx(struct e1000_adapter *adapter)
 	/* With jumbo frames, excessive C-state transition latencies result
 	 * in dropped transactions.
 	 */
-	if (adapter->netdev->mtu > ETH_DATA_LEN) {
+	if (adapter->max_frame_size > VLAN_ETH_FRAME_LEN + ETH_FCS_LEN) {
 		u32 lat =
 		    ((er32(PBA) & E1000_PBA_RXA_MASK) * 1024 -
 		     adapter->max_frame_size) * 8 / 1000;
@@ -4001,7 +4003,8 @@ void e1000e_reset(struct e1000_adapter *adapter)
 	switch (hw->mac.type) {
 	case e1000_ich9lan:
 	case e1000_ich10lan:
-		if (adapter->netdev->mtu > ETH_DATA_LEN) {
+		if (adapter->max_frame_size > VLAN_ETH_FRAME_LEN +
+					      ETH_FCS_LEN) {
 			pba = 14;
 			ew32(PBA, pba);
 			fc->high_water = 0x2800;
@@ -4020,7 +4023,8 @@ void e1000e_reset(struct e1000_adapter *adapter)
 		/* Workaround PCH LOM adapter hangs with certain network
 		 * loads.  If hangs persist, try disabling Tx flow control.
 		 */
-		if (adapter->netdev->mtu > ETH_DATA_LEN) {
+		if (adapter->max_frame_size > VLAN_ETH_FRAME_LEN +
+					      ETH_FCS_LEN) {
 			fc->high_water = 0x3500;
 			fc->low_water = 0x1500;
 		} else {
@@ -4034,7 +4038,8 @@ void e1000e_reset(struct e1000_adapter *adapter)
 	case e1000_pch_spt:
 		fc->refresh_time = 0x0400;
 
-		if (adapter->netdev->mtu <= ETH_DATA_LEN) {
+		if (adapter->max_frame_size <= VLAN_ETH_FRAME_LEN +
+					       ETH_FCS_LEN) {
 			fc->high_water = 0x05C20;
 			fc->low_water = 0x05048;
 			fc->pause_time = 0x0650;
@@ -4278,7 +4283,7 @@ void e1000e_down(struct e1000_adapter *adapter, bool reset)
 
 	/* Disable Si errata workaround on PCHx for jumbo frame flow */
 	if ((hw->mac.type >= e1000_pch2lan) &&
-	    (adapter->netdev->mtu > ETH_DATA_LEN) &&
+	    (adapter->max_frame_size > VLAN_ETH_FRAME_LEN + ETH_FCS_LEN) &&
 	    e1000_lv_jumbo_workaround_ich8lan(hw, false))
 		e_dbg("failed to disable jumbo frame workaround mode\n");
 
@@ -4391,7 +4396,8 @@ static int e1000_sw_init(struct e1000_adapter *adapter)
 
 	adapter->rx_buffer_len = VLAN_ETH_FRAME_LEN + ETH_FCS_LEN;
 	adapter->rx_ps_bsize0 = 128;
-	adapter->max_frame_size = netdev->mtu + VLAN_ETH_HLEN + ETH_FCS_LEN;
+	adapter->max_frame_size = netdev->mtu + netdev->env_hdr_len +
+				  ETH_HLEN + ETH_FCS_LEN;
 	adapter->min_frame_size = ETH_ZLEN + ETH_FCS_LEN;
 	adapter->tx_ring_count = E1000_DEFAULT_TXD;
 	adapter->rx_ring_count = E1000_DEFAULT_RXD;
@@ -5961,17 +5967,10 @@ struct rtnl_link_stats64 *e1000e_get_stats64(struct net_device *netdev,
 	return stats;
 }
 
-/**
- * e1000_change_mtu - Change the Maximum Transfer Unit
- * @netdev: network interface device structure
- * @new_mtu: new value for maximum frame size
- *
- * Returns 0 on success, negative on failure
- **/
-static int e1000_change_mtu(struct net_device *netdev, int new_mtu)
+static int e1000_change_max_frame(struct net_device *netdev, int max_frame,
+				  int new_mtu)
 {
 	struct e1000_adapter *adapter = netdev_priv(netdev);
-	int max_frame = new_mtu + VLAN_ETH_HLEN + ETH_FCS_LEN;
 
 	/* Jumbo frame support */
 	if ((max_frame > (VLAN_ETH_FRAME_LEN + ETH_FCS_LEN)) &&
@@ -5981,7 +5980,7 @@ static int e1000_change_mtu(struct net_device *netdev, int new_mtu)
 	}
 
 	/* Supported frame sizes */
-	if ((new_mtu < (VLAN_ETH_ZLEN + ETH_FCS_LEN)) ||
+	if ((new_mtu && new_mtu < (VLAN_ETH_ZLEN + ETH_FCS_LEN)) ||
 	    (max_frame > adapter->max_hw_frame_size)) {
 		e_err("Unsupported MTU setting\n");
 		return -EINVAL;
@@ -5990,7 +5989,7 @@ static int e1000_change_mtu(struct net_device *netdev, int new_mtu)
 	/* Jumbo frame workaround on 82579 and newer requires CRC be stripped */
 	if ((adapter->hw.mac.type >= e1000_pch2lan) &&
 	    !(adapter->flags2 & FLAG2_CRC_STRIPPING) &&
-	    (new_mtu > ETH_DATA_LEN)) {
+	    (max_frame > VLAN_ETH_FRAME_LEN + ETH_FCS_LEN)) {
 		e_err("Jumbo Frames not supported on this device when CRC stripping is disabled.\n");
 		return -EINVAL;
 	}
@@ -5998,9 +5997,14 @@ static int e1000_change_mtu(struct net_device *netdev, int new_mtu)
 	while (test_and_set_bit(__E1000_RESETTING, &adapter->state))
 		usleep_range(1000, 2000);
 	/* e1000e_down -> e1000e_reset dependent on max_frame_size & mtu */
+	if (new_mtu) {
+		e_info("changing MTU from %d to %d\n", netdev->mtu, new_mtu);
+		netdev->mtu = new_mtu;
+	} else {
+		e_info("changing max frame size from %d to %d\n",
+		       adapter->max_frame_size, max_frame);
+	}
 	adapter->max_frame_size = max_frame;
-	e_info("changing MTU from %d to %d\n", netdev->mtu, new_mtu);
-	netdev->mtu = new_mtu;
 
 	pm_runtime_get_sync(netdev->dev.parent);
 
@@ -6036,6 +6040,20 @@ static int e1000_change_mtu(struct net_device *netdev, int new_mtu)
 	return 0;
 }
 
+/**
+ * e1000_change_mtu - Change the Maximum Transfer Unit
+ * @netdev: network interface device structure
+ * @new_mtu: new value for maximum frame size
+ *
+ * Returns 0 on success, negative on failure
+ **/
+static int e1000_change_mtu(struct net_device *netdev, int new_mtu)
+{
+	int max_frame = new_mtu + netdev->env_hdr_len + ETH_HLEN + ETH_FCS_LEN;
+
+	return e1000_change_max_frame(netdev, max_frame, new_mtu);
+}
+
 static int e1000_mii_ioctl(struct net_device *netdev, struct ifreq *ifr,
 			   int cmd)
 {
@@ -6925,7 +6943,8 @@ static netdev_features_t e1000_fix_features(struct net_device *netdev,
 	struct e1000_hw *hw = &adapter->hw;
 
 	/* Jumbo frame workaround on 82579 and newer requires CRC be stripped */
-	if ((hw->mac.type >= e1000_pch2lan) && (netdev->mtu > ETH_DATA_LEN))
+	if ((hw->mac.type >= e1000_pch2lan) &&
+	    (adapter->max_frame_size > VLAN_ETH_FRAME_LEN + ETH_FCS_LEN))
 		features &= ~NETIF_F_RXFCS;
 
 	/* Since there is no support for separate Rx/Tx vlan accel
@@ -6977,6 +6996,20 @@ static int e1000_set_features(struct net_device *netdev,
 	return 0;
 }
 
+/**
+ * e1000_set_env_hdr_len - Set max envelope header length
+ * @netdev: network interface device structure
+ * @new_len: new value for maximum envelope header length
+ *
+ * Returns 0 on success, negative on failure
+ **/
+static int e1000_set_env_hdr_len(struct net_device *netdev, int new_len)
+{
+	int max_frame = netdev->mtu + new_len + ETH_HLEN + ETH_FCS_LEN;
+
+	return e1000_change_max_frame(netdev, max_frame, 0);
+}
+
 static const struct net_device_ops e1000e_netdev_ops = {
 	.ndo_open		= e1000e_open,
 	.ndo_stop		= e1000e_close,
@@ -6997,6 +7030,7 @@ static const struct net_device_ops e1000e_netdev_ops = {
 	.ndo_set_features = e1000_set_features,
 	.ndo_fix_features = e1000_fix_features,
 	.ndo_features_check	= passthru_features_check,
+	.ndo_set_env_hdr_len	= e1000_set_env_hdr_len,
 };
 
 /**
@@ -7119,6 +7153,8 @@ static int e1000_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 	netdev->mem_start = mmio_start;
 	netdev->mem_end = mmio_start + mmio_len;
 
+	netdev->env_hdr_len = VLAN_HLEN;
+
 	adapter->bd_number = cards_found++;
 
 	e1000e_check_options(adapter);
-- 
1.8.3.1

^ permalink raw reply related

* [PATCH RFC 0/3] Support envelope frames (802.3as)
From: Toshiaki Makita @ 2016-09-27  8:55 UTC (permalink / raw)
  To: netdev, Patrick McHardy, Stephen Hemminger, Vlad Yasevich,
	Jeff Kirsher
  Cc: Toshiaki Makita

This patch introduces software implementation of envelope frames defined
in 802.3as[1], which allows encapsulated packets to be received without
expanding MTU for them.


* Envelope frames

Envelope frames are introduces by IEEE 802.3as[1], which has been
incorpolated in IEEE 802.3-2012.

IEEE 802.3-2012 1.4.184 defines envelope frame as:
	A MAC frame that carries a Length/Type field with the Type
	interpretation that may indicate additional encapsulation
	information within the MAC client data and has a maximum length
	of 2000 octets. The envelope frame is intended to allow inclusion
	of additional prefixes and suffixes required by higher layer
	encapsulation protocols. The encapsulation protocols may use up
	to 482 octets.


* Motivation

The intended customer of this feature is mainly vlan, possibly mpls or
other encapsulation protocols.

Vlan is different than other encapsulation protocols in that the packet
size is generally larger than normal packets by vlan-header size (4 bytes).
Thus, most NICs allow packets the size of which is larger by 4 bytes
than MTU (802.3 calls this vlan-tagged packets "Q-tagged frames", whose
MTU is 1504 including vlan header. Most NICs accept Q-tagged frames).

Similarly, when doubly tagged vlan is used leveraging 802.1ad, the packet
size will be larger by vlan-header size * 2 (8 bytes). This packet size is
needed to provide Ethernet VPN transparent to the users. Thus, hardware
switches support 1508 bytes MTU when using 802.1ad, as suggested by MEF[2].
Also, Linux stacked vlan devices have 1500 bytes MTU, which emit 1508
bytes doubly tagged packets. But unfortunately some NICs don't accept 
1508 bytes packets by default, and they are dropped.

+----+ single tag +-------+ double tag +-------+ double tag +------+
|End | 1504 bytes |802.1ad| 1508 bytes |802.1ad| 1508 bytes |Linux |
|User|----------->|Edge SW|----------->|NNI SW |----------->|Server|
+----+            +-------+            +-------+     *drop* +------+
                                                     on NIC

802.3 calls such encapsulated packets larger than 1504 "envelope frames".
Most NICs lack support for envelope frames. But many of them support jumbo
frames, which can be used to implement envelope frames support in Linux.
I'm proposing this envelope frames support to fix problems described above.


* Implementation

Envelope frames require normal packets to use 1500-sized MTU, while
encapsulation headers can be added to the MTU. If we simply increase MTU
of the physical device, it causes jumbo frames as well as envelope frames
(jumbo frames are non-encapsulated packets whose MTU is larger than 1500).
So what we need here is to increase the max acceptable frame size of NICs
without changing dev->mtu.

In order to achieve this, I add a new function pointer,
.ndo_set_env_hdr_len, in net_device_ops, through which kernel can inform
device drivers of needed additional header size of envelope frames
(env_hdr_len).
Implementation in device drivers is as simple as replacing dev->mtu with
dev->mtu + env_hdr_len. This makes devices recognize dev->mtu + env_hdr_len
as MTU, and allow packets with additional header up to env_hdr_len, while
kernel networking stack recognizes dev->mtu as MTU. Thus no packets larger
than MTU will be sent other than those encapsulated by upper devices. This
effectively supports envelope frames.

Userspace API is netlink, the same as MTU. It will be a parameter which
can be configured through "ip link".


* Q&A

** Why not reducing MTU of VLAN devices?

As written in Motivation, in order to achieve transparency of Ethernet VPN,
MTU of vlan device needs to be 1500. Since this is usual in 802.1ad network,
switches in 802.1ad network send 1508-sized tagged packets. Thus, reducing
MTU of vlan device does not change the situation where Linux receives
packets whose MTU is larger than NICs' acceptable size, and does not fix
the issue.

** Why not increasing MTU of physical devices?

Increasing MTU of physical device indeed resolves the problem that NICs
cannot receive doubly tagged packets. However, this effectively allows
devices to send jumbo frames as well as envelope frames, and could cause
packet drops on network elements which does not accept jumbo frames.

** Why is .ndo_set_env_hdr_len needed?
   Why not modifying drivers to accept envelope frames by default?

Some NICs actually support envelope frames by default. One example is igb,
which always accepts packet size up to 9728.
I however don't think all NICs necessarily be able to do that since some
NICs change their behaviour when changing MTU larger than 1500.
For example, e1000e changes usage of descriptors when its MTU gets larger
than 1500. qlge also looks to change its behaviour as far as I can see from
the source code of the driver.
In order to keep the default behaviour when not using 802.1ad or stacked
vlan, some knob is needed.

** Why are drivers notified of header _length_?
   Why not introducing a knob to simply enable envelope mode?

The reason being the same as the previous question. There can be a NIC that
changes its behaviour when changing MTU larger than a certain value (I don't
know any such NICs though). If we do not notify drivers of length, the NIC
should add 482 bytes to MTU in envelope mode as defined in 802.3as, and this
may change NIC's behaviour. Vlan needs only 8 additional bytes and it may
not change NIC's behaviour. I just wanted to make it flexible to handle such
situation.
Another problem when we do not notify drivers of length is how to handle the
case where NICs partially support envelope frames. If a certain NIC does not
support 482 bytes additional header but some size of header smaller than 482,
how to handle this? If we make the .ndo_set_env_hdr_len fail in such a case,
users cannot use its additional header, even if the max acceptable size is
sufficient for them. If we make the op succeed, we need to add another API
to expose the accepted size so that users can know how long header they can
use.


* Examples

I show current behaviour of some NICs and expected behaviour when
setting env_hdr_len.

As far as I can confirm with actual equipments in our lab, there are
at least four types of drivers/devices.

(Type 1) Devices with extra buffer larger than vlan header

(Type 1-1) Devices with small amount of extra buffer
Devices/drivers that already take into account stacked vlan, or have
more extra room than it should be due to alignment restriction, etc.
This type of NICs does not require any additional operation to make
stacked vlan work.
E.g. mlx4_en, sfc

(Type 1-2) Devices with large amount of extra buffer
Devices/drivers that always accept packets up to the maximum configurable
size of MTU with jumbo frames support. This type of NICs has enough size
of extra buffer for envelope frames, the header size of which is defined
as 482 in 802.3as. They accept various types of protocols in addition to
stacked vlan.
E.g. igb

(Type 2) Devices without extra buffer larger than vlan header

(Type 2-1) Devices with generic 4 bytes extra buffer
Devices/drivers that accept MTU + 4 sized packets.
Any packets not larger than MTU + 4 are acceptable but those larger than
the value are dropped.
E.g. e1000e

(Type 2-2) Devices with 4 bytes extra buffer only for vlan
Devices/drivers that accept MTU + 4 sized packets only if it is vlan
tagged. Other packets are dropped if they exceed MTU, even if their size
is less than MTU + 4.
This type of devices even drops 802.1ad single tagged packets, if they
do not support 802.1ad vlan protocol.
E.g. bnx2x, ixgbe


The problematic NICs with stacked vlan are only type 2, but we should
assume all types of NICs can be configured with env_hdr_len, because
users do not know which NIC is which type.

The expected behaviour when users set 8 bytes env_hdr_len for stacked vlan
is as follows:
(Type 1) As they have more room than 8, do nothing.
(Type 2-1) They have 4 bytes env_hdr_len by default. When users specifies 8,
  drivers increase devices' MTU by 4 bytes.
(Type 2-2) They have 0 bytes env_hdr_len by default. When users specifies 8,
  drivers increase devices' MTU by 8 bytes.


* Recommended operations for users

- Set env_hdr_len of the physical device to 8 when they create a 802.1ad
  vlan device or stacked vlan devices.
- If setting of env_hdr_len fails (due to EOPNOTSUPP), try increasing MTU
  of the physical device.
- If increasing MTU fails, there is no other option than reducing MTU of
  vlan devices, unfortunately.

Note that those operations are not needed for 802.1q single vlan devices
at all, even after adding envelope frame support. Adjustment of MTU in case
of single 802.1q vlan should be cared by drivers as before.


* Future Plan

In the future 802.1ad vlan devices should take care of the MTU problem in
kernel and adjust env_hdr_len automatically. This can be achieved by
notifying drivers of env_hdr_len on creating 802.1ad devices.
This notification, however, does not always succeed because of lack of
.ndo support or jumbo frame feature, so this cannot completely remove
the necessity of userspace operation of env_hdr_len. Nevertheless I'm
thinking this would help users to some extent.


* Note

I submitted the previous patch set with title of "Automatic adjustment of
max frame size"[3]. Now I'm not targetting at automation but infrastracture
for the feature, as I think automation is kind of premature when even
manual operation is not possible.

This problem was discussed in Netdev 0.1:
http://www.netdevconf.org/0.1/docs/netdev01_bof_8021ad_makita_150212.pdf
This topic is also going to be discussed in Netdev 1.2:
http://www.netdevconf.org/1.2/session.html?toshiaki-makita


[1] http://www.ieee802.org/3/as/public/0607/802.3as_overview.pdf
[2] https://wiki.mef.net/display/CESG/ENNI+Attributes
[3] https://marc.info/?t=144583097100001&r=1&w=2
    https://marc.info/?t=144583097100005&r=1&w=2
    https://marc.info/?t=144583097100006&r=1&w=2
    https://marc.info/?t=144583097100004&r=1&w=2
    https://marc.info/?t=144583097100002&r=1&w=2

Toshiaki Makita (3):
  net: Add dev_set_env_hdr_len to accept envelope frames
  net: Support IFLA_ENV_HDR_LEN to configure max envelope header length
  e1000e: Add ndo_set_env_hdr_len

 drivers/net/ethernet/intel/e1000e/netdev.c | 84 +++++++++++++++++++++---------
 include/linux/netdevice.h                  | 21 ++++++++
 include/uapi/linux/if_link.h               |  1 +
 net/core/dev.c                             | 32 ++++++++++++
 net/core/rtnetlink.c                       | 16 +++++-
 5 files changed, 128 insertions(+), 26 deletions(-)

-- 
1.8.3.1

^ permalink raw reply

* Re: [PATCH] brcmfmac: implement more accurate skb tracking
From: Arend Van Spriel @ 2016-09-27  9:06 UTC (permalink / raw)
  To: Dan Williams, Rafał Miłecki
  Cc: Kalle Valo, Franky Lin, Hante Meuleman, Pieter-Paul Giesberts,
	Franky Lin,
	linux-wireless-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	open list:BROADCOM BRCM80211 IEEE802.11n WIRELESS DRIVER,
	Network Development, Linux Kernel Mailing List,
	Rafał Miłecki
In-Reply-To: <1474901966.4519.8.camel-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

On 26-9-2016 16:59, Dan Williams wrote:
> On Mon, 2016-09-26 at 14:13 +0200, Rafał Miłecki wrote:
>> On 26 September 2016 at 13:46, Arend Van Spriel
>> <arend.vanspriel-dY08KVG/lbpWk0Htik3J/w@public.gmane.org> wrote:
>>>
>>> On 26-9-2016 12:23, Rafał Miłecki wrote:
>>>>
>>>> From: Rafał Miłecki <rafal-g1n6cQUeyibVItvQsEIGlw@public.gmane.org>
>>>>
>>>> We need to track 802.1x packets to know if there are any pending
>>>> ones
>>>> for transmission. This is required for performing key update in
>>>> the
>>>> firmware.
>>>
>>> The problem we are trying to solve is a pretty old one. The problem
>>> is
>>> that wpa_supplicant uses two separate code paths: EAPOL messaging
>>> through data path and key configuration though nl80211.
>>
>> Can I find it described/reported somewhere?
> 
> If I understand the issue correctly, you can find all this in the
> supplicant code.  Once the supplicant has done whatever it wants to do
> with the data frames that just happen to be EAPOL it then sends the
> keys down to the driver with nl80211.

Indeed. EAPOL packets are simply data packets as far as the 802.11 stack
is concerned. The arrival of those in the driver is not predictable
hence we hold off the key configuration until those have been passed
over to firmware.

> But it sounds like, instead of sniffing EAPOL frames in the driver skb
> tracking and sniffing ETH_P_PAE, you should probably implement support
> for NL80211_CMD_CRIT_PROTOCOL_START/NL80211_CMD_CRIT_PROTOCOL_STOP and
> key off the passed-in NL80211_CRIT_PROTO_EAPOL.  At least at the
> beginning of connection setup only EAPOL packets will be allowed
> anyway.
> 
> It doesn't seem like the supplicant uses NL80211_CRIT_PROTO_EAPOL yet,
> but that should also be fixed in the supplicant itself.  You should
> probably get some comments from Jouni on how he'd like to see all this
> work.  But generally the less specific sniffing of frames in drivers,
> likely the better.

Indeed. That was the main motivation to introduce the CRIT_PROTO api. If
I recall correctly it was considered the task of the network manager to
issue the START/STOP. Recently noticed the use of CRIT_PROTO_DHCP on
some target system, which we already support in brcmfmac. From your
response I guess you consider CRIT_PROTO_EAPOL to be issued by the
supplicant.

Regards,
Arend

> Dan
> 
>>
>>>
>>>>
>>>> Unfortunately our old tracking code wasn't very accurate. It was
>>>> treating skb as pending as soon as it was passed by the netif.
>>>> Actual
>>>> handling packet to the firmware was happening later as brcmfmac
>>>> internally queues them and uses its own worker(s).
>>>
>>> That does not seem right. As soon as we get a 1x packet we need to
>>> wait
>>> with key configuration regardless whether it is still in the driver
>>> or
>>> handed over to firmware already.
>>
>> OK, thanks.

^ permalink raw reply

* [PATCH 4.9] brcmfmac: use correct skb freeing helper when deleting flowring
From: Rafał Miłecki @ 2016-09-27  9:14 UTC (permalink / raw)
  To: Kalle Valo
  Cc: Arend van Spriel, Franky Lin, Hante Meuleman,
	Pieter-Paul Giesberts, Franky Lin, linux-wireless,
	brcm80211-dev-list.pdl, netdev, linux-kernel,
	Rafał Miłecki

From: Rafał Miłecki <rafal@milecki.pl>

Flowrings contain skbs waiting for transmission that were passed to us
by netif. It means we checked every one of them looking for 802.1x
Ethernet type. When deleting flowring we have to use freeing function
that will check for 802.1x type as well.

Freeing skbs without a proper check was leading to counter not being
properly decreased. This was triggering a WARNING every time
brcmf_netdev_wait_pend8021x was called.

Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
---
Kalle: this isn't important enough for 4.8 as it's too late for that.

I'd like to get it for 4.9 however, as this fixes bug that could lead
to WARNING on every add_key/del_key call. We was struggling with these
WARNINGs for some time and this fixes one of two problems causing them.
---
 drivers/net/wireless/broadcom/brcm80211/brcmfmac/flowring.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/flowring.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/flowring.c
index b16b367..d0b738d 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/flowring.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/flowring.c
@@ -234,13 +234,20 @@ static void brcmf_flowring_block(struct brcmf_flowring *flow, u16 flowid,
 
 void brcmf_flowring_delete(struct brcmf_flowring *flow, u16 flowid)
 {
+	struct brcmf_bus *bus_if = dev_get_drvdata(flow->dev);
 	struct brcmf_flowring_ring *ring;
+	struct brcmf_if *ifp;
 	u16 hash_idx;
+	u8 ifidx;
 	struct sk_buff *skb;
 
 	ring = flow->rings[flowid];
 	if (!ring)
 		return;
+
+	ifidx = brcmf_flowring_ifidx_get(flow, flowid);
+	ifp = brcmf_get_ifp(bus_if->drvr, ifidx);
+
 	brcmf_flowring_block(flow, flowid, false);
 	hash_idx = ring->hash_id;
 	flow->hash[hash_idx].ifidx = BRCMF_FLOWRING_INVALID_IFIDX;
@@ -249,7 +256,7 @@ void brcmf_flowring_delete(struct brcmf_flowring *flow, u16 flowid)
 
 	skb = skb_dequeue(&ring->skblist);
 	while (skb) {
-		brcmu_pkt_buf_free_skb(skb);
+		brcmf_txfinalize(ifp, skb, false);
 		skb = skb_dequeue(&ring->skblist);
 	}
 
-- 
2.9.3

^ permalink raw reply related

* Re: [PATCH net-next V2] net/sched: pkt_cls: change tc actions order to be as the user sets
From: Hadar Hen Zion @ 2016-09-27  9:15 UTC (permalink / raw)
  To: Hadar Hen Zion
  Cc: David S. Miller, netdev, Jamal Hadi Salim, Cong Wang, Or Gerlitz
In-Reply-To: <1474963791-3717-1-git-send-email-hadarh@mellanox.com>

On Tue, Sep 27, 2016 at 11:09 AM, Hadar Hen Zion <hadarh@mellanox.com> wrote:
> Currently the created tc actions list is reversed against the order
> set by the user.
> Change the actions list order to be the same as was set by the user.
>
> This patch doesn't affect dump actions behavior.
> For dumping, action->order parameter is used so the list order doesn't
> matter.
>
> Signed-off-by: Hadar Hen Zion <hadarh@mellanox.com>
> Acked-by: Jamal Hadi Salim <jhs@mojatatu.com>


Changes from V1:
- Add a comment to the change log


> ---
>  include/net/pkt_cls.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/include/net/pkt_cls.h b/include/net/pkt_cls.h
> index 5ccaa4b..767b03a 100644
> --- a/include/net/pkt_cls.h
> +++ b/include/net/pkt_cls.h
> @@ -123,7 +123,7 @@ static inline void tcf_exts_to_list(const struct tcf_exts *exts,
>         for (i = 0; i < exts->nr_actions; i++) {
>                 struct tc_action *a = exts->actions[i];
>
> -               list_add(&a->list, actions);
> +               list_add_tail(&a->list, actions);
>         }
>  #endif
>  }
> --
> 1.8.3.1
>

^ permalink raw reply


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox