* Re: [PATCH v2 2/9] net: inet_rtm_getroute() - use new style struct initializer instead of memset
From: David Ahern @ 2018-10-01 15:25 UTC (permalink / raw)
To: Maciej Żenczykowski, Maciej Żenczykowski,
David S . Miller; +Cc: netdev
In-Reply-To: <20180930064454.187537-2-zenczykowski@gmail.com>
On 9/30/18 12:44 AM, Maciej Żenczykowski wrote:
> From: Maciej Żenczykowski <maze@google.com>
>
> Signed-off-by: Maciej Żenczykowski <maze@google.com>
> ---
> net/ipv4/route.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
Reviewed-by: David Ahern <dsahern@gmail.com>
^ permalink raw reply
* Re: [PATCH v2 6/9] net: ip6_update_pmtu() - use new style struct initializer instead of memset
From: David Ahern @ 2018-10-01 15:24 UTC (permalink / raw)
To: Maciej Żenczykowski, Maciej Żenczykowski,
David S . Miller; +Cc: netdev
In-Reply-To: <20180930064454.187537-6-zenczykowski@gmail.com>
On 9/30/18 12:44 AM, Maciej Żenczykowski wrote:
> From: Maciej Żenczykowski <maze@google.com>
>
> (allows for better compiler optimization)
>
> Signed-off-by: Maciej Żenczykowski <maze@google.com>
> ---
> net/ipv6/route.c | 17 ++++++++---------
> 1 file changed, 8 insertions(+), 9 deletions(-)
>
Reviewed-by: David Ahern <dsahern@gmail.com>
^ permalink raw reply
* Re: [PATCH v2 5/9] net: remove 1 always zero parameter from ip6_redirect_no_header()
From: David Ahern @ 2018-10-01 15:23 UTC (permalink / raw)
To: Maciej Żenczykowski, Maciej Żenczykowski,
David S . Miller; +Cc: netdev
In-Reply-To: <20180930064454.187537-5-zenczykowski@gmail.com>
On 9/30/18 12:44 AM, Maciej Żenczykowski wrote:
> From: Maciej Żenczykowski <maze@google.com>
>
> (the parameter in question is mark)
>
> Signed-off-by: Maciej Żenczykowski <maze@google.com>
> ---
> include/net/ip6_route.h | 3 +--
> net/ipv6/ndisc.c | 2 +-
> net/ipv6/route.c | 4 +---
> 3 files changed, 3 insertions(+), 6 deletions(-)
>
Reviewed-by: David Ahern <dsahern@gmail.com>
^ permalink raw reply
* Re: [PATCH v2 4/9] net: ip6_redirect_no_header() - use new style struct initializer instead of memset
From: David Ahern @ 2018-10-01 15:22 UTC (permalink / raw)
To: Maciej Żenczykowski, Maciej Żenczykowski,
David S . Miller; +Cc: netdev
In-Reply-To: <20180930064454.187537-4-zenczykowski@gmail.com>
On 9/30/18 12:44 AM, Maciej Żenczykowski wrote:
> From: Maciej Żenczykowski <maze@google.com>
>
> (allows for better compiler optimization)
>
> Signed-off-by: Maciej Żenczykowski <maze@google.com>
> ---
> net/ipv6/route.c | 17 ++++++++---------
> 1 file changed, 8 insertions(+), 9 deletions(-)
>
Reviewed-by: David Ahern <dsahern@gmail.com>
^ permalink raw reply
* Re: [PATCH v2 3/9] net: ip6_redirect() - use new style struct initializer instead of memset
From: David Ahern @ 2018-10-01 15:22 UTC (permalink / raw)
To: Maciej Żenczykowski, Maciej Żenczykowski,
David S . Miller; +Cc: netdev
In-Reply-To: <20180930064454.187537-3-zenczykowski@gmail.com>
On 9/30/18 12:44 AM, Maciej Żenczykowski wrote:
> From: Maciej Żenczykowski <maze@google.com>
>
> (allows for better compiler optimization)
>
> Signed-off-by: Maciej Żenczykowski <maze@google.com>
> ---
> net/ipv6/route.c | 19 +++++++++----------
> 1 file changed, 9 insertions(+), 10 deletions(-)
>
Reviewed-by: David Ahern <dsahern@gmail.com>
^ permalink raw reply
* Re: [PATCH v2 1/9] net: ip_rt_get_source() - use new style struct initializer instead of memset
From: David Ahern @ 2018-10-01 15:20 UTC (permalink / raw)
To: Maciej Żenczykowski, Maciej Żenczykowski,
David S . Miller; +Cc: netdev
In-Reply-To: <20180930064454.187537-1-zenczykowski@gmail.com>
On 9/30/18 12:44 AM, Maciej Żenczykowski wrote:
> From: Maciej Żenczykowski <maze@google.com>
>
> (allows for better compiler optimization)
>
> Signed-off-by: Maciej Żenczykowski <maze@google.com>
> ---
> net/ipv4/route.c | 21 +++++++++------------
> 1 file changed, 9 insertions(+), 12 deletions(-)
>
Reviewed-by: David Ahern <dsahern@gmail.com>
^ permalink raw reply
* Re: [PATCH iproute2/net-next v2] tc_util: Add support for showing TCA_STATS_BASIC_HW statistics
From: David Ahern @ 2018-10-01 15:12 UTC (permalink / raw)
To: Eelco Chaudron, Stephen Hemminger; +Cc: netdev, davem
In-Reply-To: <92F8600A-1C8F-4AF1-B84C-CCDE4120BCBA@redhat.com>
On 10/1/18 4:29 AM, Eelco Chaudron wrote:
>>> Hi Stephen, anything else required for this patch to be accepted?
>>>
>>> FYI the kernel side of this patch has been excepted on net-next.
>>>
>>> Cheers,
>>>
>>> Eelco
>>
>> David Ahern handles net-next see patchwork
>> https://patchwork.ozlabs.org/patch/956225/
>>
>> I think he was just waiting for the kernel part to merge.
>
> Thanks for making me aware of the patchwork for iproute.
>
Now that the kernel side is in, please re-send the iproute2 patches.
^ permalink raw reply
* Re: [PATCH net-next] rtnetlink: fix rtnl_fdb_dump() for shorter family headers
From: David Ahern @ 2018-10-01 15:01 UTC (permalink / raw)
To: Mauricio Faria de Oliveira; +Cc: netdev, davem
In-Reply-To: <CAO9xwp1De-XTgNYiPiXg6=BmJojXSjNZm0KofJ3neSPMD_aG3Q@mail.gmail.com>
On 10/1/18 6:44 AM, Mauricio Faria de Oliveira wrote:
>> I suspect rtnl_fdb_dump is forever stuck with the ifinfomsg struct as
>> the header if any kernel side filtering is to be done. [snip]
>
> Why exactly? I understand currently there may be little information
> to distinguish family headers, but if it comes down to certain attributes
> the function expects/uses, that can be checked if nlmsg_parse() is OK.
>
> Otherwise, if it comes down to some struct field that is not common
> between both structs, then.. well. Maybe something else/new.
struct ifinfomsg is 16 bytes; ndmsg is 12 bytes. The difference is
ifi_change in the ifinfomsg. If you don't know which header is sent, how
can you reliably parse -- and believe -- the result of the parsing?
Yes, iproute2 0's out the structs. So does FRR. But the general argument
is that userspace may not and the kernel has to this point happily
ignored fields it was not using.
The short of it is that ifi_change may be non-0 and should not be relied
on. That's the theory anyways ...
>
>> [...] As for the change
>> above, I suggest something like this:
>>
>> /* if header struct is ndmsg, no attributes can be appended */
>> if (nlmsg_len(nlh) != sizeof(struct ndmsg)) {
>> current ifinfomsg based code
>> }
>>
>
> Hm, not sure -- ndmsg can be used with attributes too in iproute2, e.g., 'dev'.
> In that case, the payload size is different/greater, and even makes
> the length check pass:
>
> $ ip --family bridge neigh
> [ 585.111034] DEBUG: rtnl_fdb_dump():3749 nlmsg_len(nlh) 12
> RTNETLINK answers: Invalid argument
> Dump terminated
>
> $ ip --family bridge neigh show dev ens3
> [ 540.231443] DEBUG: rtnl_fdb_dump():3749 nlmsg_len(nlh) 20
> [ 540.234433] DEBUG: rtnl_fdb_dump():3749 nlmsg_len(nlh) 20
> lladdr 33:33:00:00:00:01 PERMANENT
> lladdr 01:00:5e:00:00:01 PERMANENT
> lladdr 33:33:ff:e9:9d:60 PERMANENT
>
>> We certainly do not want to ignore parse failures.
>
> Well, if there are no attributes, that shouldn't be a problem,
> but unfortunately that's what happens in this case :- (
>
> Now, given the example above makes the attribute parsing pass,
> that allows the payload interpretation as ifm struct to be used.
> Fortunately this field is commonly aligned on both structs,
> and is not used in ndmsg per iproute2 ipneigh.c do_show_or_flush().
> But this might pose problems in the future if things change.
>
> If you could explain your thoughts a bit more about it, it would be
> really great.
>
Perhaps there is a work around. IFLA_MASTER is the only supported
attribute that can be appended, and it is sent as a u32. Then the
rtnl_fdb_dump function has 4 legitimate cases:
1. ndmsg = size 12 bytes
2. ndmsg + MASTER = 20 ?
3. ifinfomsg = size 16 bytes
4. ifinfomsg + MASTER = 24 ?
'ip neigh show' could send NDA_IFINDEX as an additional attribute. That
is my mistake. I should have set ndm_ifindex rather than using the
attribute, but that ship sailed 3 years ago. Anyways, that case too
might be uniquely detected. The size checks have been used in other
places, so should be ok here too.
At this point the use of ifinfomsg for dumps has created a mess
extending kernel side filtering. The point of the PROPER_HDR patch set
is to give a point in time across all dump functions where the kernel
and userspace can reliably communicate about what is wanted.
^ permalink raw reply
* Re: [net-next, PATCH 1/2, v3] net: socionext: different approach on DMA
From: Ilias Apalodimas @ 2018-10-01 14:37 UTC (permalink / raw)
To: Jesper Dangaard Brouer
Cc: netdev, jaswinder.singh, ard.biesheuvel, masami.hiramatsu, arnd,
bjorn.topel, magnus.karlsson, daniel, ast, jesus.sanchez-palencia,
vinicius.gomes, makita.toshiaki, Tariq Toukan, Tariq Toukan
In-Reply-To: <20181001154845.4cd1d5dc@redhat.com>
On Mon, Oct 01, 2018 at 03:48:45PM +0200, Jesper Dangaard Brouer wrote:
> On Mon, 1 Oct 2018 14:20:21 +0300
> Ilias Apalodimas <ilias.apalodimas@linaro.org> wrote:
>
> > On Mon, Oct 01, 2018 at 01:03:13PM +0200, Jesper Dangaard Brouer wrote:
> > > On Mon, 1 Oct 2018 12:56:58 +0300
> > > Ilias Apalodimas <ilias.apalodimas@linaro.org> wrote:
> > >
> > > > > > #2: You have allocations on the XDP fast-path.
> > > > > >
> > > > > > The REAL secret behind the XDP performance is to avoid allocations on
> > > > > > the fast-path. While I just told you to use the page-allocator and
> > > > > > order-0 pages, this will actually kill performance. Thus, to make this
> > > > > > fast, you need a driver local recycle scheme that avoids going through
> > > > > > the page allocator, which makes XDP_DROP and XDP_TX extremely fast.
> > > > > > For the XDP_REDIRECT action (which you seems to be interested in, as
> > > > > > this is needed for AF_XDP), there is a xdp_return_frame() API that can
> > > > > > make this fast.
> > > > >
> > > > > I had an initial implementation that did exactly that (that's why you the
> > > > > dma_sync_single_for_cpu() -> dma_unmap_single_attrs() is there). In the case
> > > > > of AF_XDP isn't that introducing a 'bottleneck' though? I mean you'll feed fresh
> > > > > buffers back to the hardware only when your packets have been processed from
> > > > > your userspace application
> > > >
> > > > Just a clarification here. This is the case if ZC is implemented. In my case
> > > > the buffers will be 'ok' to be passed back to the hardware once the use
> > > > userspace payload has been copied by xdp_do_redirect()
> > >
> > > Thanks for clarifying. But no, this is not introducing a 'bottleneck'
> > > for AF_XDP.
> > >
> > > For (1) the copy-mode-AF_XDP the frame (as you noticed) is "freed" or
> > > "returned" very quickly after it is copied. The code is a bit hard to
> > > follow, but in __xsk_rcv() it calls xdp_return_buff() after the memcpy.
> > > Thus, the frame can be kept DMA mapped and reused in RX-ring quickly.
> >
> > Ok makes sense. I'll send a v4 with page re-usage, while using your
> > API for page allocation
>
> Sound good, BUT do notice that using the bare page_pool, will/should
> give you increased XDP performance, but might slow-down normal network
> stack delivery, because netstack will not call xdp_return_frame() and
> instead falls back to returning the pages through the page-allocator.
>
> I'm very interested in knowing what performance increase you see with
> XDP_DROP, with just a "bare" page_pool implementation.
When i was just syncing the page fragments instead of unmap -> alloc -> map i
was getting ~340kpps (with XDP_REDIRECT). I ended up with 320kpps on this patch.
I did a couple of more changes though (like the dma mapping when allocating
the buffers) so i am not 100% sure what caused that difference
I'll let you know once i finish up the code using the API for page allocation
Regarding the change and the 'bottleneck' discussion we had. XDP_REDIRECT is
straight forward (non ZC mode). I agree with you that since the payload is
pretty much immediately copied before being flushed to the userspace, it's
unlikely you'll end up delaying the hardware (starving without buffers).
Do you think that's the same for XDP_TX? The DMA buffer will need to be synced
for the CPU, then you ring a doorbell with X packets. After that you'll have to
wait for the Tx completion and resync the buffers to the device. So you actually
make your Rx descriptors depending on your Tx completion (and keep in mind this
NIC only has 1 queue per direction)
Now for the measurements part, i'll have to check with the vendor if the
interface can do more than 340kpps and we are missing something
performance-wise.
Have you done any tests with IOMMU enabled/disabled? In theory the dma recycling
will shine against map/unmap when IOMMU is on (and the IOMMU stressed i.e have a
different NIC doing a traffic test)
>
> The mlx5 driver does not see this netstack slowdown, because it have a
> hybrid approach of maintaining a recycle ring for frames going into
> netstack, by bumping the refcnt. I think Tariq is cleaning this up.
> The mlx5 code is hard to follow... in mlx5e_xdp_handle()[1] the
> refcnt==1 and a bit is set. And in [2] the refcnt is page_ref_inc(),
> and bit is caught in [3]. (This really need to be cleaned up and
> generalized).
I've read most of the XDP related code on Intel/Mellanox
before starting my patch series. I'll have a closer look now, thanks!
>
>
>
> [1] https://github.com/torvalds/linux/blob/master/drivers/net/ethernet/mellanox/mlx5/core/en/xdp.c#L83-L88
> https://github.com/torvalds/linux/blob/v4.18/drivers/net/ethernet/mellanox/mlx5/core/en_rx.c#L952-L959
>
> [2] https://github.com/torvalds/linux/blob/v4.18/drivers/net/ethernet/mellanox/mlx5/core/en_rx.c#L1015-L1025
>
> [3] https://github.com/torvalds/linux/blob/v4.18/drivers/net/ethernet/mellanox/mlx5/core/en_rx.c#L1094-L1098
>
> --
> Best regards,
> Jesper Dangaard Brouer
> MSc.CS, Principal Kernel Engineer at Red Hat
> LinkedIn: http://www.linkedin.com/in/brouer
^ permalink raw reply
* Re: [PATCH 1/1] bridge: remove BR_GROUPFWD_RESTRICTED for arbitrary forwarding of reserved addresses
From: Richard Weinberger @ 2018-10-01 14:28 UTC (permalink / raw)
To: Stephen Hemminger
Cc: bernhard.thaler, David S. Miller, bridge, netdev, David Gstir
In-Reply-To: <20150105221034.0f69d6fd@urahara>
Stephen, Bernhard,
On Tue, Jan 6, 2015 at 7:18 AM Stephen Hemminger
<stephen@networkplumber.org> wrote:
>
> On Tue, 6 Jan 2015 01:56:15 +0100
> Bernhard Thaler <bernhard.thaler@wvnet.at> wrote:
>
> > BR_GROUPFWD_RESTRICTED bitmask restricts users from setting values to
> > /sys/class/net/brX/bridge/group_fwd_mask that allow forwarding of
> > some IEEE 802.1D Table 7-10 Reserved addresses:
> > (MAC Control) 802.3 01-80-C2-00-00-01
> > (Link Aggregation) 802.3 01-80-C2-00-00-02
> > 802.1AB LLDP 01-80-C2-00-00-0E
> > BR_GROUPFWD_RESTRICTED may have been set as an extra protection against
> > forwarding these control frames as forwarding 802.1X PAE (01-80-C2-00-00-03)
> > in 802.1X setups satisfies most common use-cases.
> > Other situations, such as placing a software based bridge as a "TAP" between two
> > devices may require to forward e.g. LLDP frames while debugging network problems
> > or actively changing/filtering traffic with ebtables.
> >
> > This patch allows to set e.g.:
> > echo 65535 > /sys/class/net/brX/bridge/group_fwd_mask
> > which sets no restrictions on the forwardable reserved addresses.
> >
> > - the default value 0 will still comply with 802.1D and not forward any
> > reserved addresses
> > - values such as 8 for forwarding 802.1X related frames will behave the
> > same way as with BR_GROUPFWD_RESTRICTED currently in place, so backward
> > compatibility to current scripts using group_fwd_masks shoudl be possible
> >
> > Administrators and network engineers however will be able to arbitrarily
> > forward any reserved addresses without BR_GROUPFWD_RESTRICTED. This will
> > be non-standard compliant behavior, but forwarding of any reserved address
> > right from the beginning is. Users should be aware of this anyway and
> > know what/why they are doing when setting values such as 65535, 32768, 16384,
> > 4, 2 for group_fwd_mask
> >
> > This patch was tested on a bridge with two interfaces created with bridge-utils.
> >
> > Signed-off-by: Bernhard Thaler <bernhard.thaler@wvnet.at>
>
> I am ok with forwarding LLDP because some people need it.
> But allowing forwarding STP or PAUSE frames is bad.
>
> We don't let people do things that break networks. Other examples
> already exist like set all 0 ethernet addresses, or the restrictions
> on allowing net 127 in IP addresses.
Sorry for unearthing this old thread, while preparing our in-house
patch I found it.
This patch does exactly what we need and already do in our network. We
have Linux bridges that
forward anything. With one restriction, the bridge has always just two slaves.
In other works, it acts like a cable. Therefore I see no problem in
forwarding STP or PAUSE frames.
Since Linux allows these days also to use 127/8 IP addresses, would
you accept a patch which allows
forwarding STP and PAUSE if the number of slaves is restricted to 2? :-)
--
Thanks,
//richard
^ permalink raw reply
* Re: __nf_register_net_hook jump label splat
From: Borislav Petkov @ 2018-10-01 20:57 UTC (permalink / raw)
To: Florian Westphal
Cc: netdev, Pablo Neira Ayuso, Jozsef Kadlecsik, David S. Miller,
netfilter-devel, coreteam, linux-kernel
In-Reply-To: <20180930183119.GB32234@zn.tnic>
On Sun, Sep 30, 2018 at 08:31:19PM +0200, Borislav Petkov wrote:
> On Sun, Sep 30, 2018 at 07:39:30PM +0200, Florian Westphal wrote:
> > Strange, these static keys have been there for years.
> > Any chance you could send your .config or bisect this?
>
> .config attached. Lemme try to bisect it, see how far I'll get.
Hmm, so I can't reproduce anymore. Maybe it was something I fat-fingered
here. In any case, rc6 + tip/master boots fine and I'll watch out in the
next days whether it really is gone.
Thx.
--
Regards/Gruss,
Boris.
Good mailing practices for 400: avoid top-posting and trim the reply.
^ permalink raw reply
* Re: [PATCH] wil6210: remove set but not used variable 'start'
From: Kalle Valo @ 2018-10-01 14:08 UTC (permalink / raw)
To: YueHaibing
Cc: Maya Erez, YueHaibing, linux-wireless, wil6210, netdev,
kernel-janitors
In-Reply-To: <1536669139-14848-1-git-send-email-yuehaibing@huawei.com>
YueHaibing <yuehaibing@huawei.com> wrote:
> Fixes gcc '-Wunused-but-set-variable' warning:
>
> drivers/net/wireless/ath/wil6210/pm.c: In function 'wil_suspend_keep_radio_on':
> drivers/net/wireless/ath/wil6210/pm.c:193:16: warning:
> variable 'start' set but not used [-Wunused-but-set-variable]
>
> Signed-off-by: YueHaibing <yuehaibing@huawei.com>
> Reviewed-by: Maya Erez <merez@codeaurora.org>
> Signed-off-by: Kalle Valo <kvalo@codeaurora.org>
Patch applied to ath-next branch of ath.git, thanks.
ec95e84c3955 wil6210: remove set but not used variable 'start'
--
https://patchwork.kernel.org/patch/10595613/
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
^ permalink raw reply
* Re: [PATCH] team: set IFF_SLAVE on team ports
From: Chas Williams @ 2018-10-01 14:06 UTC (permalink / raw)
To: Jiri Pirko, Stephen Hemminger; +Cc: Jan Blunck, LKML, netdev
In-Reply-To: <20180930093452.GG2209@nanopsycho.orion>
On 09/30/18 05:34, Jiri Pirko wrote:
> Sun, Sep 30, 2018 at 11:38:05AM CEST, stephen@networkplumber.org wrote:
>> On Sun, 30 Sep 2018 09:14:14 +0200
>> Jiri Pirko <jiri@resnulli.us> wrote:
>>
>>> Thu, Sep 27, 2018 at 04:04:26PM CEST, 3chas3@gmail.com wrote:
>>>>
>>>>
>>>> On 07/10/15 02:41, Jiri Pirko wrote:
>>>>> Thu, Jul 09, 2015 at 05:36:55PM CEST, jblunck@infradead.org wrote:
>>>>>> On Thu, Jul 9, 2015 at 12:07 PM, Jiri Pirko <jiri@resnulli.us> wrote:
>>>>>>> Thu, Jul 09, 2015 at 11:58:34AM CEST, jblunck@infradead.org wrote:
>>>>>>>> The code in net/ipv6/addrconf.c:addrconf_notify() tests for IFF_SLAVE to
>>>>>>>> decide if it should start the address configuration. Since team ports
>>>>>>>> shouldn't get link-local addresses assigned lets set IFF_SLAVE when linking
>>>>>>>> a port to the team master.
>>>>>>>
>>>>>>> I don't want to use IFF_SLAVE in team. Other master-slave devices are
>>>>>>> not using that as well, for example bridge, ovs, etc.
>>>>>>>
>>>>>>
>>>>>> Maybe they need to get fixed too. I've used that flag because it is
>>>>>> documented as
>>>>>> a "slave of a load balancer" which describes what a team port is.
>>>>>>
>>>>>>
>>>>>>> I think that this should be fixed in addrconf_notify. It should lookup
>>>>>>> if there is a master on top and bail out in that case.
>>>>>>
>>>>>> There are other virtual interfaces that have a master assigned and want to
>>>>>> participate in IPv6 address configuration.
>>>>>
>>>>> Can you give me an example?
>>>>
>>>> I would like to revisit this patch (yes, I know it has been a while). I
>>>> believe the VRF implementation uses master to group the interfaces under
>>>> a single interface.
>>>>
>>>> I don't see a reason not to use IFF_SLAVE since team and bonding are fairly
>>>> similar.
>>>
>>> Again, why do you need team port to have IFF_SLAVE flag? What do you
>>> want to achieve
>>
>> Without setting this flag IPv6 will try and make a link specific address.
>
> Why is it not an issue with bridge, ovs, and other master-slave devices?
>
It very well might be an issue for bridge and ovs. Other master-slave
devices include the existing VRF implementation in the kernel and those
slave interfaces will certainly want to use IPv6.
However, IFF_SLAVE has a specific meaning:
./include/uapi/linux/if.h: * @IFF_SLAVE: slave of a load balancer. Volatile.
The bonding driver is not the only user:
./drivers/net/eql.c:#define eql_is_slave(dev) ((dev->flags & IFF_SLAVE)
== IFF_SLAVE)
./drivers/net/eql.c: slave->dev->flags &= ~IFF_SLAVE;
./drivers/net/eql.c: slave->dev->flags |= IFF_SLAVE;
The team driver would like to use this same flag since it is a load
balancer as well. The side effect of not assigning IPv6 is a bonus.
The fact that bridges and ovs are also likely broken is a different
issue. Should there be a another flag that says "layer 2 only"? Very
possibly, but that is something all these interfaces should be using to
include bonding, team, eql, obs, bridge etc. That's not a reasonable
objection to labeling the team slave as slaves since they are literally
slaves of a load balancer.
^ permalink raw reply
* Re: [PATCH] ath9k: add reset for airtime station debugfs
From: Kalle Valo @ 2018-10-01 14:06 UTC (permalink / raw)
To: Louie Lu; +Cc: ath9k-devel, davem, linux-wireless, netdev, toke
In-Reply-To: <20180906035323.GA11337@nems_nctu_lu>
Louie Lu <git@louie.lu> wrote:
> Let user can reset station airtime status by debugfs, it will
> reset all airtime deficit to ATH_AIRTIME_QUANTUM and reset rx/tx
> airtime accumulate to 0.
>
> Signed-off-by: Louie Lu <git@louie.lu>
> Signed-off-by: Kalle Valo <kvalo@codeaurora.org>
Patch applied to ath-next branch of ath.git, thanks.
3fa8d2186550 ath9k: add reset for airtime station debugfs
--
https://patchwork.kernel.org/patch/10589835/
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
^ permalink raw reply
* Re: [net-next, PATCH 0/2, v3] net: socionext: XDP support
From: Ilias Apalodimas @ 2018-10-01 13:59 UTC (permalink / raw)
To: Björn Töpel
Cc: netdev, jaswinder.singh, ard.biesheuvel, masami.hiramatsu, arnd,
magnus.karlsson, brouer, daniel, ast, jesus.sanchez-palencia,
vinicius.gomes, makita.toshiaki
In-Reply-To: <a8745c83-574a-6e6c-ef2a-f2ae4d39d9a2@intel.com>
On Mon, Oct 01, 2018 at 02:48:54PM +0200, Björn Töpel wrote:
> On 2018-09-29 13:28, Ilias Apalodimas wrote:
> >This patch series adds AF_XDP support socionext netsec driver
>
> This series adds *XDP* support and as a result, the AF_XDP batteries
> are included. ;-)
>
Noted, thanks for cathcing this, i replaced all other AF_XDP -> XDP but seems
like i missed this one!
/Ilias
^ permalink raw reply
* Re: [net-next, PATCH 1/2, v3] net: socionext: different approach on DMA
From: Jesper Dangaard Brouer @ 2018-10-01 13:48 UTC (permalink / raw)
To: Ilias Apalodimas
Cc: netdev, jaswinder.singh, ard.biesheuvel, masami.hiramatsu, arnd,
bjorn.topel, magnus.karlsson, daniel, ast, jesus.sanchez-palencia,
vinicius.gomes, makita.toshiaki, Tariq Toukan, Tariq Toukan,
brouer
In-Reply-To: <20181001112021.GA27469@apalos>
On Mon, 1 Oct 2018 14:20:21 +0300
Ilias Apalodimas <ilias.apalodimas@linaro.org> wrote:
> On Mon, Oct 01, 2018 at 01:03:13PM +0200, Jesper Dangaard Brouer wrote:
> > On Mon, 1 Oct 2018 12:56:58 +0300
> > Ilias Apalodimas <ilias.apalodimas@linaro.org> wrote:
> >
> > > > > #2: You have allocations on the XDP fast-path.
> > > > >
> > > > > The REAL secret behind the XDP performance is to avoid allocations on
> > > > > the fast-path. While I just told you to use the page-allocator and
> > > > > order-0 pages, this will actually kill performance. Thus, to make this
> > > > > fast, you need a driver local recycle scheme that avoids going through
> > > > > the page allocator, which makes XDP_DROP and XDP_TX extremely fast.
> > > > > For the XDP_REDIRECT action (which you seems to be interested in, as
> > > > > this is needed for AF_XDP), there is a xdp_return_frame() API that can
> > > > > make this fast.
> > > >
> > > > I had an initial implementation that did exactly that (that's why you the
> > > > dma_sync_single_for_cpu() -> dma_unmap_single_attrs() is there). In the case
> > > > of AF_XDP isn't that introducing a 'bottleneck' though? I mean you'll feed fresh
> > > > buffers back to the hardware only when your packets have been processed from
> > > > your userspace application
> > >
> > > Just a clarification here. This is the case if ZC is implemented. In my case
> > > the buffers will be 'ok' to be passed back to the hardware once the use
> > > userspace payload has been copied by xdp_do_redirect()
> >
> > Thanks for clarifying. But no, this is not introducing a 'bottleneck'
> > for AF_XDP.
> >
> > For (1) the copy-mode-AF_XDP the frame (as you noticed) is "freed" or
> > "returned" very quickly after it is copied. The code is a bit hard to
> > follow, but in __xsk_rcv() it calls xdp_return_buff() after the memcpy.
> > Thus, the frame can be kept DMA mapped and reused in RX-ring quickly.
>
> Ok makes sense. I'll send a v4 with page re-usage, while using your
> API for page allocation
Sound good, BUT do notice that using the bare page_pool, will/should
give you increased XDP performance, but might slow-down normal network
stack delivery, because netstack will not call xdp_return_frame() and
instead falls back to returning the pages through the page-allocator.
I'm very interested in knowing what performance increase you see with
XDP_DROP, with just a "bare" page_pool implementation.
The mlx5 driver does not see this netstack slowdown, because it have a
hybrid approach of maintaining a recycle ring for frames going into
netstack, by bumping the refcnt. I think Tariq is cleaning this up.
The mlx5 code is hard to follow... in mlx5e_xdp_handle()[1] the
refcnt==1 and a bit is set. And in [2] the refcnt is page_ref_inc(),
and bit is caught in [3]. (This really need to be cleaned up and
generalized).
[1] https://github.com/torvalds/linux/blob/master/drivers/net/ethernet/mellanox/mlx5/core/en/xdp.c#L83-L88
https://github.com/torvalds/linux/blob/v4.18/drivers/net/ethernet/mellanox/mlx5/core/en_rx.c#L952-L959
[2] https://github.com/torvalds/linux/blob/v4.18/drivers/net/ethernet/mellanox/mlx5/core/en_rx.c#L1015-L1025
[3] https://github.com/torvalds/linux/blob/v4.18/drivers/net/ethernet/mellanox/mlx5/core/en_rx.c#L1094-L1098
--
Best regards,
Jesper Dangaard Brouer
MSc.CS, Principal Kernel Engineer at Red Hat
LinkedIn: http://www.linkedin.com/in/brouer
^ permalink raw reply
* Re: [PATCH net] rhashtable: prevent work queue schedule while dismantling
From: Herbert Xu @ 2018-10-01 13:40 UTC (permalink / raw)
To: Eric Dumazet; +Cc: David S . Miller, netdev, Eric Dumazet, Thomas Graf
In-Reply-To: <20181001131627.163099-1-edumazet@google.com>
On Mon, Oct 01, 2018 at 06:16:27AM -0700, Eric Dumazet wrote:
> syszbot found an interesting use-after-free [1] happening
> while IPv4 fragment rhashtable was destroyed at netns dismantle.
>
> While no insertions can possibly happen at the time a dismantling
> netns is destroying this rhashtable, timers can still fire and
> attempt to remove elements from this rhashtable.
Hmm, I think that's your real problem. rhashtable_free_and_destroy
doesn't take any locks with respect to the normal insertion/removal
path so it definitely isn't safe to call it while you're still
invoking the normal rhashtable remove function.
Cheers,
--
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply
* [PATCH net] rhashtable: prevent work queue schedule while dismantling
From: Eric Dumazet @ 2018-10-01 13:16 UTC (permalink / raw)
To: David S . Miller
Cc: netdev, Eric Dumazet, Eric Dumazet, Thomas Graf, Herbert Xu
syszbot found an interesting use-after-free [1] happening
while IPv4 fragment rhashtable was destroyed at netns dismantle.
While no insertions can possibly happen at the time a dismantling
netns is destroying this rhashtable, timers can still fire and
attempt to remove elements from this rhashtable.
Since automatic shrinking is enabled, this might then schedule
the work queue to perform the shrinking from process context,
which was not expected.
I have thought of simply forcing ht->p.automatic_shrinking to
false before the cancel_work_sync(&ht->run_work);
in rhashtable_free_and_destroy(), but that would be still racy.
Lets use a separate boolean to not only prevent the work queue
being scheduled, but also make sure rht_deferred_worker() would
return early.
[1]
BUG: KASAN: use-after-free in __read_once_size include/linux/compiler.h:188 [inline]
BUG: KASAN: use-after-free in rhashtable_last_table+0x216/0x240 lib/rhashtable.c:217
Read of size 8 at addr ffff88019a4c8840 by task kworker/0:4/8279
CPU: 0 PID: 8279 Comm: kworker/0:4 Not tainted 4.19.0-rc5+ #61
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
Workqueue: events rht_deferred_worker
Call Trace:
__dump_stack lib/dump_stack.c:77 [inline]
dump_stack+0x1c4/0x2b4 lib/dump_stack.c:113
print_address_description.cold.8+0x9/0x1ff mm/kasan/report.c:256
kasan_report_error mm/kasan/report.c:354 [inline]
kasan_report.cold.9+0x242/0x309 mm/kasan/report.c:412
__asan_report_load8_noabort+0x14/0x20 mm/kasan/report.c:433
__read_once_size include/linux/compiler.h:188 [inline]
rhashtable_last_table+0x216/0x240 lib/rhashtable.c:217
rht_deferred_worker+0x157/0x1de0 lib/rhashtable.c:410
process_one_work+0xc90/0x1b90 kernel/workqueue.c:2153
worker_thread+0x17f/0x1390 kernel/workqueue.c:2296
kthread+0x35a/0x420 kernel/kthread.c:246
ret_from_fork+0x3a/0x50 arch/x86/entry/entry_64.S:413
Allocated by task 5:
save_stack+0x43/0xd0 mm/kasan/kasan.c:448
set_track mm/kasan/kasan.c:460 [inline]
kasan_kmalloc+0xc7/0xe0 mm/kasan/kasan.c:553
__do_kmalloc_node mm/slab.c:3682 [inline]
__kmalloc_node+0x47/0x70 mm/slab.c:3689
kmalloc_node include/linux/slab.h:555 [inline]
kvmalloc_node+0xb9/0xf0 mm/util.c:423
kvmalloc include/linux/mm.h:577 [inline]
kvzalloc include/linux/mm.h:585 [inline]
bucket_table_alloc+0x9a/0x4e0 lib/rhashtable.c:176
rhashtable_rehash_alloc+0x73/0x100 lib/rhashtable.c:353
rht_deferred_worker+0x278/0x1de0 lib/rhashtable.c:413
process_one_work+0xc90/0x1b90 kernel/workqueue.c:2153
worker_thread+0x17f/0x1390 kernel/workqueue.c:2296
kthread+0x35a/0x420 kernel/kthread.c:246
ret_from_fork+0x3a/0x50 arch/x86/entry/entry_64.S:413
Freed by task 8283:
save_stack+0x43/0xd0 mm/kasan/kasan.c:448
set_track mm/kasan/kasan.c:460 [inline]
__kasan_slab_free+0x102/0x150 mm/kasan/kasan.c:521
kasan_slab_free+0xe/0x10 mm/kasan/kasan.c:528
__cache_free mm/slab.c:3498 [inline]
kfree+0xcf/0x230 mm/slab.c:3813
kvfree+0x61/0x70 mm/util.c:452
bucket_table_free+0xda/0x250 lib/rhashtable.c:108
rhashtable_free_and_destroy+0x152/0x900 lib/rhashtable.c:1163
inet_frags_exit_net+0x3d/0x50 net/ipv4/inet_fragment.c:96
ipv4_frags_exit_net+0x73/0x90 net/ipv4/ip_fragment.c:914
ops_exit_list.isra.7+0xb0/0x160 net/core/net_namespace.c:153
cleanup_net+0x555/0xb10 net/core/net_namespace.c:551
process_one_work+0xc90/0x1b90 kernel/workqueue.c:2153
worker_thread+0x17f/0x1390 kernel/workqueue.c:2296
kthread+0x35a/0x420 kernel/kthread.c:246
ret_from_fork+0x3a/0x50 arch/x86/entry/entry_64.S:413
The buggy address belongs to the object at ffff88019a4c8800
which belongs to the cache kmalloc-16384 of size 16384
The buggy address is located 64 bytes inside of
16384-byte region [ffff88019a4c8800, ffff88019a4cc800)
The buggy address belongs to the page:
page:ffffea0006693200 count:1 mapcount:0 mapping:ffff8801da802200 index:0x0 compound_mapcount: 0
flags: 0x2fffc0000008100(slab|head)
raw: 02fffc0000008100 ffffea0006685608 ffffea0006617c08 ffff8801da802200
raw: 0000000000000000 ffff88019a4c8800 0000000100000001 0000000000000000
page dumped because: kasan: bad access detected
Memory state around the buggy address:
ffff88019a4c8700: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
ffff88019a4c8780: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
>ffff88019a4c8800: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
^
ffff88019a4c8880: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
ffff88019a4c8900: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
Signed-off-by: Eric Dumazet <edumazet@google.com>
Reported-by: syzbot <syzkaller@googlegroups.com>
Cc: Thomas Graf <tgraf@suug.ch>
Cc: Herbert Xu <herbert@gondor.apana.org.au>
---
include/linux/rhashtable-types.h | 2 ++
include/linux/rhashtable.h | 1 +
lib/rhashtable.c | 25 ++++++++++++++-----------
3 files changed, 17 insertions(+), 11 deletions(-)
diff --git a/include/linux/rhashtable-types.h b/include/linux/rhashtable-types.h
index 763d613ce2c2f275037c3b1b5ce4d390394295c6..03d86e79dfd664c48c5e437c609deb62cbc33180 100644
--- a/include/linux/rhashtable-types.h
+++ b/include/linux/rhashtable-types.h
@@ -75,6 +75,7 @@ struct rhashtable_params {
* @max_elems: Maximum number of elements in table
* @p: Configuration parameters
* @rhlist: True if this is an rhltable
+ * @dead: True if in dismantle phase
* @run_work: Deferred worker to expand/shrink asynchronously
* @mutex: Mutex to protect current/future table swapping
* @lock: Spin lock to protect walker list
@@ -86,6 +87,7 @@ struct rhashtable {
unsigned int max_elems;
struct rhashtable_params p;
bool rhlist;
+ bool dead;
struct work_struct run_work;
struct mutex mutex;
spinlock_t lock;
diff --git a/include/linux/rhashtable.h b/include/linux/rhashtable.h
index eb71110392479784db1d4a0b9d86d2eee6631789..5efc7d5cc66ba37c70c7e3d5ff98473df6a3837e 100644
--- a/include/linux/rhashtable.h
+++ b/include/linux/rhashtable.h
@@ -934,6 +934,7 @@ static inline int __rhashtable_remove_fast_one(
if (err > 0) {
atomic_dec(&ht->nelems);
if (unlikely(ht->p.automatic_shrinking &&
+ !ht->dead &&
rht_shrink_below_30(ht, tbl)))
schedule_work(&ht->run_work);
err = 0;
diff --git a/lib/rhashtable.c b/lib/rhashtable.c
index 30526afa8343124f06f0649592ee246dc4d88fbe..99f9aa5a768d849d9afe3f50f25c300c37d9c07d 100644
--- a/lib/rhashtable.c
+++ b/lib/rhashtable.c
@@ -406,19 +406,21 @@ static void rht_deferred_worker(struct work_struct *work)
ht = container_of(work, struct rhashtable, run_work);
mutex_lock(&ht->mutex);
- tbl = rht_dereference(ht->tbl, ht);
- tbl = rhashtable_last_table(ht, tbl);
+ if (!ht->dead) {
+ tbl = rht_dereference(ht->tbl, ht);
+ tbl = rhashtable_last_table(ht, tbl);
- if (rht_grow_above_75(ht, tbl))
- err = rhashtable_rehash_alloc(ht, tbl, tbl->size * 2);
- else if (ht->p.automatic_shrinking && rht_shrink_below_30(ht, tbl))
- err = rhashtable_shrink(ht);
- else if (tbl->nest)
- err = rhashtable_rehash_alloc(ht, tbl, tbl->size);
-
- if (!err)
- err = rhashtable_rehash_table(ht);
+ if (rht_grow_above_75(ht, tbl))
+ err = rhashtable_rehash_alloc(ht, tbl, tbl->size * 2);
+ else if (ht->p.automatic_shrinking &&
+ rht_shrink_below_30(ht, tbl))
+ err = rhashtable_shrink(ht);
+ else if (tbl->nest)
+ err = rhashtable_rehash_alloc(ht, tbl, tbl->size);
+ if (!err)
+ err = rhashtable_rehash_table(ht);
+ }
mutex_unlock(&ht->mutex);
if (err)
@@ -1138,6 +1140,7 @@ void rhashtable_free_and_destroy(struct rhashtable *ht,
struct bucket_table *tbl, *next_tbl;
unsigned int i;
+ ht->dead = true;
cancel_work_sync(&ht->run_work);
mutex_lock(&ht->mutex);
--
2.19.0.605.g01d371f741-goog
^ permalink raw reply related
* Re: [PATCH] dt-bindings: can: rcar_can: Add r8a7744 support
From: Simon Horman @ 2018-10-01 12:56 UTC (permalink / raw)
To: Biju Das
Cc: Wolfgang Grandegger, Marc Kleine-Budde, Rob Herring, Mark Rutland,
David S. Miller, linux-can, netdev, devicetree,
Geert Uytterhoeven, Chris Paterson, Fabrizio Castro,
linux-renesas-soc
In-Reply-To: <1538050128-47356-1-git-send-email-biju.das@bp.renesas.com>
On Thu, Sep 27, 2018 at 01:08:48PM +0100, Biju Das wrote:
> Document RZ/G1N (r8a7744) SoC specific bindings.
>
> Signed-off-by: Biju Das <biju.das@bp.renesas.com>
> Reviewed-by: Chris Paterson <Chris.Paterson2@renesas.com>
Reviewed-by: Simon Horman <horms+renesas@verge.net.au>
^ permalink raw reply
* Re: [PATCH 1/1] macsec: reflect the master interface state
From: Sabrina Dubroca @ 2018-10-01 12:52 UTC (permalink / raw)
To: Radu Rendec; +Cc: netdev, davem, ptalbert
In-Reply-To: <CAD5jUk8gpiX_AyiRPf7fnBouyxWS7V8FEAiiug1A3iMdr_YJBA@mail.gmail.com>
2018-09-19, 12:44:41 -0400, Radu Rendec wrote:
> Hello,
Gah, sorry, this fell into the "week-end" crack and I forgot to answer :(
> On Wed, Sep 19, 2018 at 11:24 AM Sabrina Dubroca <sd@queasysnail.net> wrote:
> > 2018-09-18, 20:16:12 -0400, Radu Rendec wrote:
> > > This patch makes macsec interfaces reflect the state of the underlying
> > > interface: if the master interface changes state to up/down, the macsec
> > > interface changes its state accordingly.
> >
> > Yes, that's a bit unfortunate. I was wondering if we should just allow
> > setting the device up, and let link state handle the rest.
>
> Yes, that could work. It would also be consistent with the idea of
> propagating only the link state.
Do you want to handle it, or should I?
> > It's missing small parts of link state handling that I have been
> > testing, mainly when creating a new device.
>
> Can you please be more specific? I've just looked at macsec_newlink()
> and I didn't notice anything related to link state handling.
Yes, that's actually the problem. For example,
macvlan_common_newlink() does:
netif_stacked_transfer_operstate(lowerdev, dev);
linkwatch_fire_event(dev);
If you try to create a macsec device UP with its lowerdev UP:
ip link set $lower up
ip link add link $lower up type macsec
You'll get:
macsec0@$lower: <BROADCAST,MULTICAST,UP,LOWER_UP> [...] state UNKNOWN [...]
and you want "state UP".
> > My version of the patch only does netif_stacked_transfer_operstate(),
> > and skips setting the device administratively down (ie, same as
> > NETDEV_CHANGE).
>
> That makes sense. But, since you mentioned you had your own patch, does
> it make sense for me to continue working on mine?
Here's what I have (without a commit message, because I hadn't written
one yet -- yours looks pretty good, other than missing a "Fixes:" tag):
diff --git a/drivers/net/macsec.c b/drivers/net/macsec.c
index 7de88b33d5b9..3532cdee2761 100644
--- a/drivers/net/macsec.c
+++ b/drivers/net/macsec.c
@@ -3308,6 +3308,9 @@ static int macsec_newlink(struct net *net, struct net_device *dev,
if (err < 0)
goto del_dev;
+ netif_stacked_transfer_operstate(real_dev, dev);
+ linkwatch_fire_event(dev);
+
macsec_generation++;
return 0;
@@ -3492,6 +3495,20 @@ static int macsec_notify(struct notifier_block *this, unsigned long event,
return NOTIFY_DONE;
switch (event) {
+ case NETDEV_DOWN:
+ case NETDEV_UP:
+ case NETDEV_CHANGE: {
+ struct macsec_dev *m, *n;
+ struct macsec_rxh_data *rxd;
+
+ rxd = macsec_data_rtnl(real_dev);
+ list_for_each_entry_safe(m, n, &rxd->secys, secys) {
+ struct net_device *dev = m->secy.netdev;
+
+ netif_stacked_transfer_operstate(real_dev, dev);
+ }
+ break;
+ }
case NETDEV_UNREGISTER: {
struct macsec_dev *m, *n;
struct macsec_rxh_data *rxd;
If you want to keep working on this, that's okay for me. I'm not hung
up on who gets authorship.
> > > }
> > > + case NETDEV_UP:
> > > + list_for_each_entry(m, &rxd->secys, secys) {
> > > + struct net_device *dev = m->secy.netdev;
> > > + int flags = dev_get_flags(dev);
> > > +
> > > + if (!(flags & IFF_UP)) {
> > > + dev_change_flags(dev, flags | IFF_UP);
> > > + netif_stacked_transfer_operstate(real_dev, dev);
> > > + }
> > > + }
> > > + break;
> >
> > I don't like that this completely ignores whether the device was done
> > independently of the lower link. Maybe the administrator actually
> > wants the macsec device down, regardless of state changes on the lower
> > device.
>
> I thought about that too and I don't like it either. Perhaps the vlan
> approach of having a "loose binding" flag is worth considering. Then the
> admin can decice for themselves.
Do you have a use case where you'd want that? If that solves some
problem for you (a problem that can't be solved just by fixing the
current bug), then ok, we can consider it. I prefer to avoid adding
obscure options and more code unless they're necessary.
> However, I guess the problem disappears if only the link state is
> propagated. The only caveat to that is to not propagate an "up" link
> state while the macsec interface is administratively down, but
> "remember" to propagate it later if the macsec interface is
> administratively set to "up".
>
> Thanks for the feedback!
And sorry for the delay in answering :/
--
Sabrina
^ permalink raw reply related
* [PATCH bpf-next v2 4/5] ethtool: don't allow disabling queues with umem installed
From: Magnus Karlsson @ 2018-10-01 12:51 UTC (permalink / raw)
To: magnus.karlsson, bjorn.topel, ast, daniel, netdev, jakub.kicinski
In-Reply-To: <1538398297-14862-1-git-send-email-magnus.karlsson@intel.com>
From: Jakub Kicinski <jakub.kicinski@netronome.com>
We already check the RSS indirection table does not use queues which
would be disabled by channel reconfiguration. Make sure user does not
try to disable queues which have a UMEM and zero-copy AF_XDP socket
installed.
Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
Reviewed-by: Quentin Monnet <quentin.monnet@netronome.com>
---
include/net/xdp_sock.h | 7 +++++++
net/core/ethtool.c | 11 +++++++++++
net/xdp/xdp_umem.c | 4 ++--
3 files changed, 20 insertions(+), 2 deletions(-)
diff --git a/include/net/xdp_sock.h b/include/net/xdp_sock.h
index 70a115b..13acb98 100644
--- a/include/net/xdp_sock.h
+++ b/include/net/xdp_sock.h
@@ -86,6 +86,7 @@ struct xdp_umem_fq_reuse *xsk_reuseq_prepare(u32 nentries);
struct xdp_umem_fq_reuse *xsk_reuseq_swap(struct xdp_umem *umem,
struct xdp_umem_fq_reuse *newq);
void xsk_reuseq_free(struct xdp_umem_fq_reuse *rq);
+struct xdp_umem *xdp_get_umem_from_qid(struct net_device *dev, u16 queue_id);
static inline char *xdp_umem_get_data(struct xdp_umem *umem, u64 addr)
{
@@ -183,6 +184,12 @@ static inline void xsk_reuseq_free(struct xdp_umem_fq_reuse *rq)
{
}
+static inline struct xdp_umem *xdp_get_umem_from_qid(struct net_device *dev,
+ u16 queue_id)
+{
+ return NULL;
+}
+
static inline char *xdp_umem_get_data(struct xdp_umem *umem, u64 addr)
{
return NULL;
diff --git a/net/core/ethtool.c b/net/core/ethtool.c
index 9a648fb..5a788ad 100644
--- a/net/core/ethtool.c
+++ b/net/core/ethtool.c
@@ -27,6 +27,7 @@
#include <linux/rtnetlink.h>
#include <linux/sched/signal.h>
#include <linux/net.h>
+#include <net/xdp_sock.h>
/*
* Some useful ethtool_ops methods that're device independent.
@@ -1656,7 +1657,9 @@ static noinline_for_stack int ethtool_set_channels(struct net_device *dev,
void __user *useraddr)
{
struct ethtool_channels channels, curr = { .cmd = ETHTOOL_GCHANNELS };
+ u16 from_channel, to_channel;
u32 max_rx_in_use = 0;
+ unsigned int i;
if (!dev->ethtool_ops->set_channels || !dev->ethtool_ops->get_channels)
return -EOPNOTSUPP;
@@ -1680,6 +1683,14 @@ static noinline_for_stack int ethtool_set_channels(struct net_device *dev,
(channels.combined_count + channels.rx_count) <= max_rx_in_use)
return -EINVAL;
+ /* Disabling channels, query zero-copy AF_XDP sockets */
+ from_channel = channels.combined_count +
+ min(channels.rx_count, channels.tx_count);
+ to_channel = curr.combined_count + max(curr.rx_count, curr.tx_count);
+ for (i = from_channel; i < to_channel; i++)
+ if (xdp_get_umem_from_qid(dev, i))
+ return -EINVAL;
+
return dev->ethtool_ops->set_channels(dev, &channels);
}
diff --git a/net/xdp/xdp_umem.c b/net/xdp/xdp_umem.c
index 4d6c665..773326f 100644
--- a/net/xdp/xdp_umem.c
+++ b/net/xdp/xdp_umem.c
@@ -55,8 +55,8 @@ static void xdp_reg_umem_at_qid(struct net_device *dev, struct xdp_umem *umem,
dev->_tx[queue_id].umem = umem;
}
-static struct xdp_umem *xdp_get_umem_from_qid(struct net_device *dev,
- u16 queue_id)
+struct xdp_umem *xdp_get_umem_from_qid(struct net_device *dev,
+ u16 queue_id)
{
if (queue_id < dev->real_num_rx_queues)
return dev->_rx[queue_id].umem;
--
2.7.4
^ permalink raw reply related
* [PATCH bpf-next v2 5/5] xsk: simplify xdp_clear_umem_at_qid implementation
From: Magnus Karlsson @ 2018-10-01 12:51 UTC (permalink / raw)
To: magnus.karlsson, bjorn.topel, ast, daniel, netdev, jakub.kicinski
In-Reply-To: <1538398297-14862-1-git-send-email-magnus.karlsson@intel.com>
As we now do not allow ethtool to deactivate the queue id we are
running an AF_XDP socket on, we can simplify the implementation of
xdp_clear_umem_at_qid().
Signed-off-by: Magnus Karlsson <magnus.karlsson@intel.com>
---
net/xdp/xdp_umem.c | 7 ++-----
1 file changed, 2 insertions(+), 5 deletions(-)
diff --git a/net/xdp/xdp_umem.c b/net/xdp/xdp_umem.c
index 773326f..c6007c5 100644
--- a/net/xdp/xdp_umem.c
+++ b/net/xdp/xdp_umem.c
@@ -68,12 +68,9 @@ struct xdp_umem *xdp_get_umem_from_qid(struct net_device *dev,
static void xdp_clear_umem_at_qid(struct net_device *dev, u16 queue_id)
{
- /* Zero out the entry independent on how many queues are configured
- * at this point in time, as it might be used in the future.
- */
- if (queue_id < dev->num_rx_queues)
+ if (queue_id < dev->real_num_rx_queues)
dev->_rx[queue_id].umem = NULL;
- if (queue_id < dev->num_tx_queues)
+ if (queue_id < dev->real_num_tx_queues)
dev->_tx[queue_id].umem = NULL;
}
--
2.7.4
^ permalink raw reply related
* [PATCH bpf-next v2 3/5] ethtool: rename local variable max -> curr
From: Magnus Karlsson @ 2018-10-01 12:51 UTC (permalink / raw)
To: magnus.karlsson, bjorn.topel, ast, daniel, netdev, jakub.kicinski
In-Reply-To: <1538398297-14862-1-git-send-email-magnus.karlsson@intel.com>
From: Jakub Kicinski <jakub.kicinski@netronome.com>
ethtool_set_channels() validates the config against driver's max
settings. It retrieves the current config and stores it in a
variable called max. This was okay when only max settings were
accessed but we will soon want to access current settings as
well, so calling the entire structure max makes the code less
readable.
While at it drop unnecessary parenthesis.
Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
Reviewed-by: Quentin Monnet <quentin.monnet@netronome.com>
---
net/core/ethtool.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/net/core/ethtool.c b/net/core/ethtool.c
index 96afc55..9a648fb 100644
--- a/net/core/ethtool.c
+++ b/net/core/ethtool.c
@@ -1655,7 +1655,7 @@ static noinline_for_stack int ethtool_get_channels(struct net_device *dev,
static noinline_for_stack int ethtool_set_channels(struct net_device *dev,
void __user *useraddr)
{
- struct ethtool_channels channels, max = { .cmd = ETHTOOL_GCHANNELS };
+ struct ethtool_channels channels, curr = { .cmd = ETHTOOL_GCHANNELS };
u32 max_rx_in_use = 0;
if (!dev->ethtool_ops->set_channels || !dev->ethtool_ops->get_channels)
@@ -1664,13 +1664,13 @@ static noinline_for_stack int ethtool_set_channels(struct net_device *dev,
if (copy_from_user(&channels, useraddr, sizeof(channels)))
return -EFAULT;
- dev->ethtool_ops->get_channels(dev, &max);
+ dev->ethtool_ops->get_channels(dev, &curr);
/* ensure new counts are within the maximums */
- if ((channels.rx_count > max.max_rx) ||
- (channels.tx_count > max.max_tx) ||
- (channels.combined_count > max.max_combined) ||
- (channels.other_count > max.max_other))
+ if (channels.rx_count > curr.max_rx ||
+ channels.tx_count > curr.max_tx ||
+ channels.combined_count > curr.max_combined ||
+ channels.other_count > curr.max_other)
return -EINVAL;
/* ensure the new Rx count fits within the configured Rx flow
--
2.7.4
^ permalink raw reply related
* [PATCH bpf-next v2 2/5] xsk: fix bug when trying to use both copy and zero-copy on one queue id
From: Magnus Karlsson @ 2018-10-01 12:51 UTC (permalink / raw)
To: magnus.karlsson, bjorn.topel, ast, daniel, netdev, jakub.kicinski
In-Reply-To: <1538398297-14862-1-git-send-email-magnus.karlsson@intel.com>
Previously, the xsk code did not record which umem was bound to a
specific queue id. This was not required if all drivers were zero-copy
enabled as this had to be recorded in the driver anyway. So if a user
tried to bind two umems to the same queue, the driver would say
no. But if copy-mode was first enabled and then zero-copy mode (or the
reverse order), we mistakenly enabled both of them on the same umem
leading to buggy behavior. The main culprit for this is that we did
not store the association of umem to queue id in the copy case and
only relied on the driver reporting this. As this relation was not
stored in the driver for copy mode (it does not rely on the AF_XDP
NDOs), this obviously could not work.
This patch fixes the problem by always recording the umem to queue id
relationship in the netdev_queue and netdev_rx_queue structs. This way
we always know what kind of umem has been bound to a queue id and can
act appropriately at bind time.
Signed-off-by: Magnus Karlsson <magnus.karlsson@intel.com>
---
net/xdp/xdp_umem.c | 90 ++++++++++++++++++++++++++++++++++++++----------------
net/xdp/xdp_umem.h | 2 +-
net/xdp/xsk.c | 7 -----
3 files changed, 64 insertions(+), 35 deletions(-)
diff --git a/net/xdp/xdp_umem.c b/net/xdp/xdp_umem.c
index 555427b..4d6c665 100644
--- a/net/xdp/xdp_umem.c
+++ b/net/xdp/xdp_umem.c
@@ -42,27 +42,47 @@ void xdp_del_sk_umem(struct xdp_umem *umem, struct xdp_sock *xs)
}
}
-int xdp_umem_query(struct net_device *dev, u16 queue_id)
+/* The umem is stored both in the _rx struct and the _tx struct as we do
+ * not know if the device has more tx queues than rx, or the opposite.
+ * This might also change during run time.
+ */
+static void xdp_reg_umem_at_qid(struct net_device *dev, struct xdp_umem *umem,
+ u16 queue_id)
{
- struct netdev_bpf bpf;
+ if (queue_id < dev->real_num_rx_queues)
+ dev->_rx[queue_id].umem = umem;
+ if (queue_id < dev->real_num_tx_queues)
+ dev->_tx[queue_id].umem = umem;
+}
- ASSERT_RTNL();
+static struct xdp_umem *xdp_get_umem_from_qid(struct net_device *dev,
+ u16 queue_id)
+{
+ if (queue_id < dev->real_num_rx_queues)
+ return dev->_rx[queue_id].umem;
+ if (queue_id < dev->real_num_tx_queues)
+ return dev->_tx[queue_id].umem;
- memset(&bpf, 0, sizeof(bpf));
- bpf.command = XDP_QUERY_XSK_UMEM;
- bpf.xsk.queue_id = queue_id;
+ return NULL;
+}
- if (!dev->netdev_ops->ndo_bpf)
- return 0;
- return dev->netdev_ops->ndo_bpf(dev, &bpf) ?: !!bpf.xsk.umem;
+static void xdp_clear_umem_at_qid(struct net_device *dev, u16 queue_id)
+{
+ /* Zero out the entry independent on how many queues are configured
+ * at this point in time, as it might be used in the future.
+ */
+ if (queue_id < dev->num_rx_queues)
+ dev->_rx[queue_id].umem = NULL;
+ if (queue_id < dev->num_tx_queues)
+ dev->_tx[queue_id].umem = NULL;
}
int xdp_umem_assign_dev(struct xdp_umem *umem, struct net_device *dev,
- u32 queue_id, u16 flags)
+ u16 queue_id, u16 flags)
{
bool force_zc, force_copy;
struct netdev_bpf bpf;
- int err;
+ int err = 0;
force_zc = flags & XDP_ZEROCOPY;
force_copy = flags & XDP_COPY;
@@ -70,17 +90,23 @@ int xdp_umem_assign_dev(struct xdp_umem *umem, struct net_device *dev,
if (force_zc && force_copy)
return -EINVAL;
- if (force_copy)
- return 0;
+ rtnl_lock();
+ if (xdp_get_umem_from_qid(dev, queue_id)) {
+ err = -EBUSY;
+ goto out_rtnl_unlock;
+ }
- if (!dev->netdev_ops->ndo_bpf || !dev->netdev_ops->ndo_xsk_async_xmit)
- return force_zc ? -EOPNOTSUPP : 0; /* fail or fallback */
+ xdp_reg_umem_at_qid(dev, umem, queue_id);
+ umem->dev = dev;
+ umem->queue_id = queue_id;
+ if (force_copy)
+ /* For copy-mode, we are done. */
+ goto out_rtnl_unlock;
- rtnl_lock();
- err = xdp_umem_query(dev, queue_id);
- if (err) {
- err = err < 0 ? -EOPNOTSUPP : -EBUSY;
- goto err_rtnl_unlock;
+ if (!dev->netdev_ops->ndo_bpf ||
+ !dev->netdev_ops->ndo_xsk_async_xmit) {
+ err = -EOPNOTSUPP;
+ goto err_unreg_umem;
}
bpf.command = XDP_SETUP_XSK_UMEM;
@@ -89,18 +115,20 @@ int xdp_umem_assign_dev(struct xdp_umem *umem, struct net_device *dev,
err = dev->netdev_ops->ndo_bpf(dev, &bpf);
if (err)
- goto err_rtnl_unlock;
+ goto err_unreg_umem;
rtnl_unlock();
dev_hold(dev);
- umem->dev = dev;
- umem->queue_id = queue_id;
umem->zc = true;
return 0;
-err_rtnl_unlock:
+err_unreg_umem:
+ xdp_clear_umem_at_qid(dev, queue_id);
+ if (!force_zc)
+ err = 0; /* fallback to copy mode */
+out_rtnl_unlock:
rtnl_unlock();
- return force_zc ? err : 0; /* fail or fallback */
+ return err;
}
static void xdp_umem_clear_dev(struct xdp_umem *umem)
@@ -108,7 +136,7 @@ static void xdp_umem_clear_dev(struct xdp_umem *umem)
struct netdev_bpf bpf;
int err;
- if (umem->dev) {
+ if (umem->zc) {
bpf.command = XDP_SETUP_XSK_UMEM;
bpf.xsk.umem = NULL;
bpf.xsk.queue_id = umem->queue_id;
@@ -119,9 +147,17 @@ static void xdp_umem_clear_dev(struct xdp_umem *umem)
if (err)
WARN(1, "failed to disable umem!\n");
+ }
+
+ if (umem->dev) {
+ rtnl_lock();
+ xdp_clear_umem_at_qid(umem->dev, umem->queue_id);
+ rtnl_unlock();
+ }
+ if (umem->zc) {
dev_put(umem->dev);
- umem->dev = NULL;
+ umem->zc = false;
}
}
diff --git a/net/xdp/xdp_umem.h b/net/xdp/xdp_umem.h
index c8be1ad..2760322 100644
--- a/net/xdp/xdp_umem.h
+++ b/net/xdp/xdp_umem.h
@@ -9,7 +9,7 @@
#include <net/xdp_sock.h>
int xdp_umem_assign_dev(struct xdp_umem *umem, struct net_device *dev,
- u32 queue_id, u16 flags);
+ u16 queue_id, u16 flags);
bool xdp_umem_validate_queues(struct xdp_umem *umem);
void xdp_get_umem(struct xdp_umem *umem);
void xdp_put_umem(struct xdp_umem *umem);
diff --git a/net/xdp/xsk.c b/net/xdp/xsk.c
index 5a432df..caeddad1 100644
--- a/net/xdp/xsk.c
+++ b/net/xdp/xsk.c
@@ -419,13 +419,6 @@ static int xsk_bind(struct socket *sock, struct sockaddr *addr, int addr_len)
}
qid = sxdp->sxdp_queue_id;
-
- if ((xs->rx && qid >= dev->real_num_rx_queues) ||
- (xs->tx && qid >= dev->real_num_tx_queues)) {
- err = -EINVAL;
- goto out_unlock;
- }
-
flags = sxdp->sxdp_flags;
if (flags & XDP_SHARED_UMEM) {
--
2.7.4
^ permalink raw reply related
* [PATCH bpf-next v2 0/5] xsk: fix bug when trying to use both copy and zero-copy mode
From: Magnus Karlsson @ 2018-10-01 12:51 UTC (permalink / raw)
To: magnus.karlsson, bjorn.topel, ast, daniel, netdev, jakub.kicinski
Previously, the xsk code did not record which umem was bound to a
specific queue id. This was not required if all drivers were zero-copy
enabled as this had to be recorded in the driver anyway. So if a user
tried to bind two umems to the same queue, the driver would say
no. But if copy-mode was first enabled and then zero-copy mode (or the
reverse order), we mistakenly enabled both of them on the same umem
leading to buggy behavior. The main culprit for this is that we did
not store the association of umem to queue id in the copy case and
only relied on the driver reporting this. As this relation was not
stored in the driver for copy mode (it does not rely on the AF_XDP
NDOs), this obviously could not work.
This patch fixes the problem by always recording the umem to queue id
relationship in the netdev_queue and netdev_rx_queue structs. This way
we always know what kind of umem has been bound to a queue id and can
act appropriately at bind time. To make the bind semantics consistent
with ethtool queue manipulations and to facilitate the implementation
of drivers, we also forbid decreasing the number of queues/channels
with ethtool if there is an active AF_XDP socket in the set of queues
that are disabled.
Jakub, please take a look at your patches. The last one I had to
change slightly to make it fit with the new interface
xdp_get_umem_from_qid(). An added bonus with this function is that we,
in the future, can also use it from the driver to get a umem, thus
simplifying driver implementations (and later remove the umem from the
NDO completely). Björn will mail patches, at a later point in time,
using this in the i40e and ixgbe drivers, that removes a good chunk of
code from the ZC implementations. I also made your code aware of Tx
queues. If we create a socket that only has a Tx queue, then the queue
id will refer to a Tx queue id only and could be larger than the
available amount of Rx queues. Please take a look at it.
Differences against v1:
* Included patches from Jakub that forbids decreasing the number of active
queues if a queue to be deactivated has an AF_XDP socket. These have
been adapted somewhat to the new interfaces in patch 2.
* Removed redundant check against real_num_[rt]x_queue in xsk_bind
* Only need to test against real_num_[rt]x_queues in
xdp_clear_umem_at_qid.
Patch 1: Introduces a umem reference in the netdev_rx_queue and
netdev_queue structs.
Patch 2: Records which queue_id is bound to which umem and make sure
that you cannot bind two different umems to the same queue_id.
Patch 3: Pre patch to ethtool_set_channels.
Patch 4: Forbid decreasing the number of active queues if a deactivated
queue has an AF_XDP socket.
Patch 5: Simplify xdp_clear_umem_at_qid now when ethtool cannot deactivate
the queue id we are running on.
I based this patch set on bpf-next commit 5bf7a60b8e70 ("bpf: permit
CGROUP_DEVICE programs accessing helper bpf_get_current_cgroup_id()")
Thanks: Magnus
Jakub Kicinski (2):
ethtool: rename local variable max -> curr
ethtool: don't allow disabling queues with umem installed
Magnus Karlsson (3):
net: add umem reference in netdev{_rx}_queue
xsk: fix bug when trying to use both copy and zero-copy on one queue
id
xsk: simplify xdp_clear_umem_at_qid implementation
include/linux/netdevice.h | 6 ++++
include/net/xdp_sock.h | 7 ++++
net/core/ethtool.c | 23 +++++++++----
net/xdp/xdp_umem.c | 87 ++++++++++++++++++++++++++++++++---------------
net/xdp/xdp_umem.h | 2 +-
net/xdp/xsk.c | 7 ----
6 files changed, 91 insertions(+), 41 deletions(-)
^ 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