* Re: [PATCH net] rhashtable: prevent work queue schedule while dismantling
From: Eric Dumazet @ 2018-10-01 16:25 UTC (permalink / raw)
To: Herbert Xu; +Cc: David Miller, netdev, Eric Dumazet, Thomas Graf
In-Reply-To: <20181001134022.jpcunwro5nvvrbkr@gondor.apana.org.au>
On Mon, Oct 1, 2018 at 6:40 AM Herbert Xu <herbert@gondor.apana.org.au> wrote:
>
> 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.
>
Ah... for some reason I thought it was ok to delete elems while
rhashtable_free_and_destroy() was running.
I would love not having to use a rhashtable_walk...() to first expunge
the hash table
before rhashtable_destroy()
^ permalink raw reply
* Re: [PATCH 1/1] bridge: remove BR_GROUPFWD_RESTRICTED for arbitrary forwarding of reserved addresses
From: Florian Fainelli @ 2018-10-01 16:24 UTC (permalink / raw)
To: Richard Weinberger, Stephen Hemminger
Cc: bernhard.thaler, David S. Miller, bridge, netdev, David Gstir
In-Reply-To: <CAFLxGvzVbWvQ5YA8NS-zcESguNNvJAbE_99SdFcAjbftCXzsdQ@mail.gmail.com>
On 10/01/2018 07:28 AM, Richard Weinberger wrote:
> 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.
If all you are doing is forwarding anything, one thing I experimented
with before is the following:
# tc qdisc add dev eth1 handle ffff: ingress
# tc qdisc add dev eth3 handle ffff: ingress
# tc filter add dev eth3 parent ffff: u32 \
> match u32 0 0 \
> action mirred egress redirect dev eth1
# tc filter add dev eth1 parent ffff: u32 \
> match u32 0 0 \
> action mirred egress redirect dev eth3
# ifconfig eth3 promisc
# ifconfig eth1 promisc
and this works just fine actually, bypassing the bridge layer entirely.
>
> 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? :-)
>
--
Florian
^ permalink raw reply
* Re: [PATCH] cxgb4: Use proper enum in IEEE_FAUX_SYNC
From: Nick Desaulniers @ 2018-10-01 22:50 UTC (permalink / raw)
To: Nathan Chancellor; +Cc: ganeshgr, David S. Miller, netdev, LKML
In-Reply-To: <20181001034738.29475-1-natechancellor@gmail.com>
On Sun, Sep 30, 2018 at 8:47 PM Nathan Chancellor
<natechancellor@gmail.com> wrote:
>
> Clang warns when one enumerated type is implicitly converted to another.
>
> drivers/net/ethernet/chelsio/cxgb4/cxgb4_dcb.c:390:4: warning: implicit
> conversion from enumeration type 'enum cxgb4_dcb_state' to different
> enumeration type 'enum cxgb4_dcb_state_input' [-Wenum-conversion]
Luckily these 2 enums had the same values, so this change is no
functional change. Thanks for the patch!
Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>
> IEEE_FAUX_SYNC(dev, dcb);
> ^~~~~~~~~~~~~~~~~~~~~~~~
> drivers/net/ethernet/chelsio/cxgb4/cxgb4_dcb.h:70:10: note: expanded
> from macro 'IEEE_FAUX_SYNC'
> CXGB4_DCB_STATE_FW_ALLSYNCED);
> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
>
> Use the equivalent value of the expected type to silence Clang while
> resulting in no functional change.
>
> CXGB4_DCB_STATE_FW_ALLSYNCED = CXGB4_DCB_INPUT_FW_ALLSYNCED = 3
>
> Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
> ---
> drivers/net/ethernet/chelsio/cxgb4/cxgb4_dcb.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_dcb.h b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_dcb.h
> index 02040b99c78a..484ee8290090 100644
> --- a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_dcb.h
> +++ b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_dcb.h
> @@ -67,7 +67,7 @@
> do { \
> if ((__dcb)->dcb_version == FW_PORT_DCB_VER_IEEE) \
> cxgb4_dcb_state_fsm((__dev), \
> - CXGB4_DCB_STATE_FW_ALLSYNCED); \
> + CXGB4_DCB_INPUT_FW_ALLSYNCED); \
> } while (0)
>
> /* States we can be in for a port's Data Center Bridging.
> --
> 2.19.0
>
--
Thanks,
~Nick Desaulniers
^ permalink raw reply
* Re: [net-next, PATCH 1/2, v3] net: socionext: different approach on DMA
From: Jesper Dangaard Brouer @ 2018-10-01 15:58 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: <20181001143706.GB810@apalos>
On Mon, 1 Oct 2018 17:37:06 +0300
Ilias Apalodimas <ilias.apalodimas@linaro.org> wrote:
> 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 explicitly asked for the XDP_DROP performance... because it will tell
me/us if your hardware is actually the bottleneck. For your specific
hardware, you might be limited by the cost of DMA-sync. It might be
faster to use the DMA-map/unmap calls(?).
I'm hinting you should take one step at a time, and measure. Knowing
and identifying the limits, is essential. Else you are doing blind
optimizations. If you don't know the HW limit, then you don't know what
the gap is to optimum (and then you don't know when to stop optimizing).
> 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)
The page_pool will cache-pages (it have a pool of pages) that should
large enough to handle some pages are outstanding in the TX completion
queue.
>
> 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.
Yes, please. The XDP_DROP test I requested above is exactly an attempt
to determine what the NIC HW limits are... esle you are working blindly.
> 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)
Nope, I could not get the IOMMU working on my testlab, last time I
tried to activate it. Hench, why I have not implemented/optimized DMA
map/unmap stuff too much (e.g. mlx5 currently does a DMA unmap for
XDP_REDIRECT, which should be fixed).
> > 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: [net-next 1/8] ice: use [sr]q.count when checking if queue is initialized
From: Keller, Jacob E @ 2018-10-01 15:52 UTC (permalink / raw)
To: Sergei Shtylyov, Kirsher, Jeffrey T, davem@davemloft.net
Cc: netdev@vger.kernel.org, nhorman@redhat.com, sassmann@redhat.com,
jogreene@redhat.com, Venkataramanan, Anirudh
In-Reply-To: <08825e96-a15e-b7bc-fb12-fdd951508be8@cogentembedded.com>
> -----Original Message-----
> From: netdev-owner@vger.kernel.org [mailto:netdev-owner@vger.kernel.org]
> On Behalf Of Sergei Shtylyov
> Sent: Friday, September 28, 2018 2:02 AM
> To: Kirsher, Jeffrey T <jeffrey.t.kirsher@intel.com>; davem@davemloft.net
> Cc: Keller, Jacob E <jacob.e.keller@intel.com>; netdev@vger.kernel.org;
> nhorman@redhat.com; sassmann@redhat.com; jogreene@redhat.com;
> Venkataramanan, Anirudh <anirudh.venkataramanan@intel.com>
> Subject: Re: [net-next 1/8] ice: use [sr]q.count when checking if queue is
> initialized
>
> Hello!
>
> On 9/27/2018 7:21 PM, Jeff Kirsher wrote:
>
> > From: Jacob Keller <jacob.e.keller@intel.com>
> >
> > When shutting down the controlqs, we check if they are initialized
> > before we shut them down and destroy the lock. This is important, as it
> > prevents attempts to access the lock of an already shutdown queue.
> >
> > Unfortunately, we checked rq.head and sq.head as the value to determine
> > if the queue was initialized. This doesn't work, because head is not
> > reset when the queue is shutdown. In some flows, the adminq will have
> > already been shut down prior to calling ice_shutdown_all_ctrlqs. This
> > can result in a crash due to attempting to access the already destroyed
> > mutex.
> >
> > Fix this by using rq.count and sq.count instead. Indeed, ice_shutdown_sq
> > and ice_shutdown_rq already indicate that this is the value we should be
> > using to determine of the queue was initialized.
>
> s/of/if/?
Yes. However, unless there's a functional change required as well, I don't think it's worth fixing this.
Thanks,
Jake
>
> > Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
> > Signed-off-by: Anirudh Venkataramanan
> <anirudh.venkataramanan@intel.com>
> > Tested-by: Andrew Bowers <andrewx.bowers@intel.com>
> > Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
> [...]
>
> MBR, Sergei
^ permalink raw reply
* Re: [PATCH net-next v3 1/2] netlink: ipv4 igmp join notifications
From: Roopa Prabhu @ 2018-10-01 15:38 UTC (permalink / raw)
To: Patrick Ruddy
Cc: David Ahern, netdev, Jiří Pírko, Stephen Hemminger,
Nikolay Aleksandrov
In-Reply-To: <CAJieiUgHPj-oBVKHqFc4sEGe0s7VUomohQk8DGGvdrp5tm98Hw@mail.gmail.com>
On Wed, Sep 26, 2018 at 10:23 AM Roopa Prabhu <roopa@cumulusnetworks.com> wrote:
>
> On Tue, Sep 25, 2018 at 2:34 AM, Patrick Ruddy
> <pruddy@vyatta.att-mail.com> wrote:
> > On Wed, 2018-09-19 at 21:47 -0700, David Ahern wrote:
> >> On 9/18/18 6:12 AM, Patrick Ruddy wrote:
> >> >
> >> > I've hit a small snag with adding the new groups. The number of defined
> >> > groups currently sits at 31 so I can only add one before hitting the
> >>
> >> I believe you have no more available. RTNLGRP_* has been defined from 0
> >> (RTNLGRP_NONE) to 31 (RTNLGRP_IPV6_MROUTE_R) which covers the u32 range.
> >>
> >> > limit defined by the 32 bit groups bitmask in socakddr_nl. I can use 1
> >> > group for both v4 and v6 notifications which seems like the sensible
> >> > options since the AF is carried separately, but it breaks the precedent
> >> > where there are separate IPV4 and IPV6 groups for IFADDR.
> >> >
> >> > I have the combined group patches ready and can share them if that's
> >> > the preference.
> >> >
> >> > Has there been any previous discussion about extending the number of
> >> > availabel groups?
> >> >
> >>
> >> I have not tried it, but from a prior code review I believe you have you
> >> use setsockopt to add groups > 31.
> >
> > I can certainly join the new groups using setsockopt and
> > NETLINK_ADD_MEMBERSHIP.
> > I can't see any examples of extending the defined group list within the
> > kernel so I assume I just add to the RTNLGRP enum list with a suitable
> > comment to indicate that later groups must be joined with the mechanism
> > above or am I missing some other way of dynamically adding groups?
> >
>
> With a quick look, there are other subsystem specific groups:
> xfrm_nlgroups, nfnetlink_groups ...which i see apps registering using
> NETLINK_ADD_MEMBERSHIP.
>
scratch that. These groups are for different netlink protocols and the
limit on netlink groups per protocol seems to be 32.
We seem to have hit the max on groups for NETLINK_ROUTE protocol. we
will have to rework the group handling
to make room for more groups. We do need room for more groups in the
future and not just for this patchset.
> seems like an overkill to add something like this for your case.
>
> yet another option to consider:
> use family: RTNL_FAMILY_IPMR/ RTNL_FAMILY_IP6MR with RTM_GETADDR/DELADDR
> and use the existing groups: RTNLGRP_IPV4_IFADDR / RTNLGRP_IPV6_IFADDR
>
> (pls check if this will break any existing users)
>
> precedence is ipmr fib rules.
^ permalink raw reply
* Re: [PATCH net-next] rtnetlink: fix rtnl_fdb_dump() for shorter family headers
From: Mauricio Faria de Oliveira @ 2018-10-01 15:38 UTC (permalink / raw)
To: David Ahern; +Cc: netdev, davem
In-Reply-To: <9fd4bf6f-47d1-4178-d16e-ec3b3d099ae3@gmail.com>
On Mon, Oct 1, 2018 at 12:01 PM David Ahern <dsahern@gmail.com> wrote:
>
> 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.
Right, I see the point of the unknown header type, and wanted to understand
whether you had more reasons in mind, as I imagined the particular
users/attributes
involved in this function could have provided an way out.
*But*, as you pointed out, the general case is that userspace can do anything.
So, now I see -- the assumption for particular cases is not an option. Thanks.
> The short of it is that ifi_change may be non-0 and should not be relied
> on. That's the theory anyways ...
And/or ndm_ifindex, which in the ifinfomsg type cast, gives brport_idx
a non-zero value
I don't yet know if it's equivalent, but if not, that's a problem.
(i.e., specify a non-brport thing, but it's processed as such)
> <snip>
> 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.
Ok, thanks for your suggestions.
I'll do some research/learning on them, and give it a try for a v2.
> 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.
Nice. I guess for this particular problem/thread, we'll won't be able to
use that one, since existing/older userspace that doesn't know about
it should be fixed for.. but definitely a great thing that should help in
this general problem.
Thank you,
--
Mauricio Faria de Oliveira
^ permalink raw reply
* Re: [PATCH v2] ath5k: Remove unused BUG_ON
From: Kalle Valo @ 2018-10-01 15:36 UTC (permalink / raw)
To: Nathan Chancellor
Cc: Jiri Slaby, Nick Kossifidis, Luis R. Rodriguez, linux-wireless,
netdev, linux-kernel, Nathan Chancellor
In-Reply-To: <20180921193235.1723-1-natechancellor@gmail.com>
Nathan Chancellor <natechancellor@gmail.com> wrote:
> Clang warns that the address of a pointer will always evaluated as true
> in a boolean context:
>
> drivers/net/wireless/ath/ath5k/debug.c:1031:14: warning: address of
> array 'ah->sbands' will always evaluate to 'true'
> [-Wpointer-bool-conversion]
> BUG_ON(!ah->sbands);
> ~~~~~^~~~~~
> ./include/asm-generic/bug.h:61:45: note: expanded from macro 'BUG_ON'
> #define BUG_ON(condition) do { if (unlikely(condition)) BUG(); } while (0)
> ^~~~~~~~~
> ./include/linux/compiler.h:77:42: note: expanded from macro 'unlikely'
> # define unlikely(x) __builtin_expect(!!(x), 0)
> ^
> 1 warning generated.
>
> Given that this condition is always false because of the logical not,
> just remove it.
>
> Link: https://github.com/ClangBuiltLinux/linux/issues/130
> Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
Patch applied to wireless-drivers-next.git, thanks.
0891c7f4a253 ath5k: Remove unused BUG_ON
--
https://patchwork.kernel.org/patch/10610881/
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
^ permalink raw reply
* Re: [PATCH v2 7/9] net: rtmsg_to_fib6_config() - use new style struct initializer instead of memset
From: David Ahern @ 2018-10-01 15:36 UTC (permalink / raw)
To: Maciej Żenczykowski, Maciej Żenczykowski,
David S . Miller; +Cc: netdev
In-Reply-To: <20180930064454.187537-7-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 | 32 ++++++++++++++++----------------
> 1 file changed, 16 insertions(+), 16 deletions(-)
>
Reviewed-by: David Ahern <dsahern@gmail.com>
^ permalink raw reply
* Re: [PATCH v2 8/9] net: rtm_to_fib6_config() - use new style struct initializer instead of memset
From: David Ahern @ 2018-10-01 15:35 UTC (permalink / raw)
To: Maciej Żenczykowski, Maciej Żenczykowski,
David S . Miller; +Cc: netdev
In-Reply-To: <20180930064454.187537-8-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 | 23 ++++++++++++-----------
> 1 file changed, 12 insertions(+), 11 deletions(-)
>
Reviewed-by: David Ahern <dsahern@gmail.com>
^ permalink raw reply
* Re: [PATCH v2 9/9] net: inet6_rtm_getroute() - use new style struct initializer instead of memset
From: David Ahern @ 2018-10-01 15:26 UTC (permalink / raw)
To: Maciej Żenczykowski, Maciej Żenczykowski,
David S . Miller; +Cc: netdev
In-Reply-To: <20180930064454.187537-9-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/ipv6/route.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
Reviewed-by: David Ahern <dsahern@gmail.com>
^ permalink raw reply
* 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
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