* Re: [PATCH 2/2] [RESEND] nfp: avoid using getnstimeofday64()
From: David Miller @ 2018-07-12 21:56 UTC (permalink / raw)
To: arnd
Cc: jakub.kicinski, john.hurley, simon.horman, pieter.jansenvanvuuren,
jiri, oss-drivers, netdev, linux-kernel
In-Reply-To: <20180711123003.453442-2-arnd@arndb.de>
From: Arnd Bergmann <arnd@arndb.de>
Date: Wed, 11 Jul 2018 14:29:53 +0200
> getnstimeofday64 is deprecated in favor of the ktime_get() family of
> functions. The direct replacement would be ktime_get_real_ts64(),
> but I'm picking the basic ktime_get() instead:
>
> - using a ktime_t simplifies the code compared to timespec64
> - using monotonic time instead of real time avoids issues caused
> by a concurrent settimeofday() or during a leap second adjustment.
>
> Acked-by: Jakub Kicinski <jakub.kicinski@netronome.com>
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Applied to net-next.
^ permalink raw reply
* Re: [PATCH net] net: systemport: Fix CRC forwarding check for SYSTEMPORT Lite
From: David Miller @ 2018-07-12 21:47 UTC (permalink / raw)
To: f.fainelli; +Cc: netdev, linux-kernel
In-Reply-To: <20180711094758.1670-1-f.fainelli@gmail.com>
From: Florian Fainelli <f.fainelli@gmail.com>
Date: Wed, 11 Jul 2018 02:47:58 -0700
> SYSTEMPORT Lite reversed the logic compared to SYSTEMPORT, the
> GIB_FCS_STRIP bit is set when the Ethernet FCS is stripped, and that bit
> is not set by default. Fix the logic such that we properly check whether
> that bit is set or not and we don't forward an extra 4 bytes to the
> network stack.
>
> Fixes: 44a4524c54af ("net: systemport: Add support for SYSTEMPORT Lite")
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
Applied.
^ permalink raw reply
* [PATCH net] net/ipv6: Do not allow device only routes via the multipath API
From: dsahern @ 2018-07-12 21:48 UTC (permalink / raw)
To: netdev; +Cc: eric.dumazet, David Ahern
From: David Ahern <dsahern@gmail.com>
Eric reported that reverting the patch that fixed and simplified IPv6
multipath routes means reverting back to invalid userspace notifications.
eg.,
$ ip -6 route add 2001:db8:1::/64 nexthop dev eth0 nexthop dev eth1
only generates a single notification:
2001:db8:1::/64 dev eth0 metric 1024 pref medium
While working on a fix for this problem I found another case that is just
broken completely - a multipath route with a gateway followed by device
followed by gateway:
$ ip -6 ro add 2001:db8:103::/64
nexthop via 2001:db8:1::64
nexthop dev dummy2
nexthop via 2001:db8:3::64
In this case the device only route is dropped completely - no notification
to userpsace but no addition to the FIB either:
$ ip -6 ro ls
2001:db8:1::/64 dev dummy1 proto kernel metric 256 pref medium
2001:db8:2::/64 dev dummy2 proto kernel metric 256 pref medium
2001:db8:3::/64 dev dummy3 proto kernel metric 256 pref medium
2001:db8:103::/64 metric 1024
nexthop via 2001:db8:1::64 dev dummy1 weight 1
nexthop via 2001:db8:3::64 dev dummy3 weight 1 pref medium
fe80::/64 dev dummy1 proto kernel metric 256 pref medium
fe80::/64 dev dummy2 proto kernel metric 256 pref medium
fe80::/64 dev dummy3 proto kernel metric 256 pref medium
Really, IPv6 multipath is just FUBAR'ed beyond repair when it comes to
device only routes, so do not allow it all.
This change will break any scripts relying on the mpath api for insert,
but I don't see any other way to handle the permutations. Besides, since
the routes are added to the FIB as standalone (non-multipath) routes the
kernel is not doing what the user requested, so it might as well tell the
user that.
Reported-by: Eric Dumazet <eric.dumazet@gmail.com>
Signed-off-by: David Ahern <dsahern@gmail.com>
---
net/ipv6/route.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/net/ipv6/route.c b/net/ipv6/route.c
index 63f99411f0de..1f1f0f318d74 100644
--- a/net/ipv6/route.c
+++ b/net/ipv6/route.c
@@ -4388,6 +4388,13 @@ static int ip6_route_multipath_add(struct fib6_config *cfg,
rt = NULL;
goto cleanup;
}
+ if (!rt6_qualify_for_ecmp(rt)) {
+ err = EINVAL;
+ NL_SET_ERR_MSG(extack,
+ "Device only routes can not be added for IPv6 using the multipath API.");
+ fib6_info_release(rt);
+ goto cleanup;
+ }
rt->fib6_nh.nh_weight = rtnh->rtnh_hops + 1;
--
2.11.0
^ permalink raw reply related
* Re: [PATCH mlx5-next v1 2/8] net/mlx5: Add support for flow table destination number
From: Or Gerlitz @ 2018-07-12 21:51 UTC (permalink / raw)
To: Jason Gunthorpe
Cc: Yishai Hadas, Doug Ledford, Leon Romanovsky, RDMA mailing list,
Saeed Mahameed, linux-netdev
In-Reply-To: <20180712212618.GS30390@mellanox.com>
On Fri, Jul 13, 2018 at 12:26 AM, Jason Gunthorpe <jgg@mellanox.com> wrote:
> On Fri, Jul 13, 2018 at 12:00:41AM +0300, Or Gerlitz wrote:
>> On Wed, Jul 11, 2018 at 2:10 PM, Leon Romanovsky <leon@kernel.org> wrote:
>> > From: Yishai Hadas <yishaih@mellanox.com>
>> >
>> > Add support to set a destination from a flow table number.
>> > This functionality will be used in downstream patches from this
>> > series by the DEVX stuff.
>>
>> Reading your cover letter, I still don't understand what is missing
>> in the current mlx5 fs core API for your needs. After all, you do
>> create flow tables from the IB driver through fs core calls, right?
>> so @ the end of the day, you have the FT pointer to provide the
>> core, why you need the FT number?
>
> Via the devx API userspace can create flow tables directly without
> going to the driver's flow steering core.
so why you change the core?
^ permalink raw reply
* Re: [PATCH net 0/2] sfc: filter locking fixes
From: David Miller @ 2018-07-12 21:52 UTC (permalink / raw)
To: bkenward; +Cc: netdev, linux-net-drivers
In-Reply-To: <ffa1a8d2-7906-29f9-6089-f968aa376681@solarflare.com>
From: Bert Kenward <bkenward@solarflare.com>
Date: Wed, 11 Jul 2018 11:39:39 +0100
> Two fixes for sfc ef10 filter table locking. Initially spotted
> by lockdep, but one issue has also been seen in normal use.
Series applied, thanks.
^ permalink raw reply
* Re: [PATCH net-next v2 0/2] net/sched: act_skbedit: lockless data path
From: David Miller @ 2018-07-12 21:54 UTC (permalink / raw)
To: dcaratti; +Cc: xiyou.wangcong, netdev
In-Reply-To: <cover.1531315251.git.dcaratti@redhat.com>
From: Davide Caratti <dcaratti@redhat.com>
Date: Wed, 11 Jul 2018 16:04:48 +0200
> the data path of act_skbedit can be faster if we avoid using spinlocks:
> - patch 1 converts act_skbedit statistics to use per-cpu counters
> - patch 2 lets act_skbedit use RCU to read/update its configuration
>
> test procedure (using pktgen from https://github.com/netoptimizer):
>
> # ip link add name eth1 type dummy
> # ip link set dev eth1 up
> # tc qdisc add dev eth1 clsact
> # tc filter add dev eth1 egress matchall action skbedit priority c1a0:c1a0
> # for c in 1 2 4 ; do
> > ./pktgen_bench_xmit_mode_queue_xmit.sh -v -s 64 -t $c -n 5000000 -i eth1
> > done
>
> test results (avg. pps/thread)
>
> $c | before patch | after patch | improvement
> ----+--------------+--------------+------------
> 1 | 3917464 ± 3% | 4000458 ± 3% | irrelevant
> 2 | 3455367 ± 4% | 3953076 ± 1% | +14%
> 4 | 2496594 ± 2% | 3801123 ± 3% | +52%
>
> v2: rebased on latest net-next
Series applied, thank you.
^ permalink raw reply
* Re: [BUG] bonded interfaces drop bpdu (stp) frames
From: Jay Vosburgh @ 2018-07-12 22:03 UTC (permalink / raw)
To: =?UTF-8?B?TWFoZXNoIEJhbmRld2FyICjgpK7gpLngpYfgpLYg4KSs4KSC4KSh4KWH4KS14KS+4KSwKQ==?=
Cc: Michal Soltys, Chonggang Li, linux-netdev
In-Reply-To: <CAF2d9jjXmiVZwDu-q+M5bmdrgyMr0U_tiUzxoCUiWwZT4NS5Cw@mail.gmail.com>
Mahesh Bandewar (महेश बंडेवार) wrote:
>On Thu, Jul 12, 2018 at 11:03 AM, Jay Vosburgh
><jay.vosburgh@canonical.com> wrote:
>> Michal Soltys <soltys@ziu.info> wrote:
>>
>>>On 07/12/2018 04:51 PM, Jay Vosburgh wrote:
>>>> Mahesh Bandewar (महेश बंडेवार) wrote:
>>>>
>>>>> On Wed, Jul 11, 2018 at 3:23 PM, Michal Soltys <soltys@ziu.info> wrote:
>>>>>>
>>>>>> Hi,
>>>>>>
>>>>>> As weird as that sounds, this is what I observed today after bumping
>>>>>> kernel version. I have a setup where 2 bonds are attached to linux
>>>>>> bridge and physically are connected to two switches doing MSTP (and
>>>>>> linux bridge is just passing them).
>>>>>>
>>>>>> Initially I suspected some changes related to bridge code - but quick
>>>>>> peek at the code showed nothing suspicious - and the part of it that
>>>>>> explicitly passes stp frames if stp is not enabled has seen little
>>>>>> changes (e.g. per-port group_fwd_mask added recently). Furthermore - if
>>>>>> regular non-bonded interfaces are attached everything works fine.
>>>>>>
>>>>>> Just to be sure I detached the bond (802.3ad mode) and checked it with
>>>>>> simple tcpdump (ether proto \\stp) - and indeed no hello packets were
>>>>>> there (with them being present just fine on active enslaved interface,
>>>>>> or on the bond device in earlier kernels).
>>>>>>
>>>>>> If time permits I'll bisect tommorow to pinpoint the commit, but from
>>>>>> quick todays test - 4.9.x is working fine, while 4.16.16 (tested on
>>>>>> debian) and 4.17.3 (tested on archlinux) are failing.
>>>>>>
>>>>>> Unless this is already a known issue (or you have any suggestions what
>>>>>> could be responsible).
>>>>>>
>>>>> I believe these are link-local-multicast messages and sometime back a
>>>>> change went into to not pass those frames to the bonding master. This
>>>>> could be the side effect of that.
>>>>
>>>> Mahesh, I suspect you're thinking of:
>>>>
>>>> commit b89f04c61efe3b7756434d693b9203cc0cce002e
>>>> Author: Chonggang Li <chonggangli@google.com>
>>>> Date: Sun Apr 16 12:02:18 2017 -0700
>>>>
>>>> bonding: deliver link-local packets with skb->dev set to link that packets arrived on
>>>>
>>>> Michal, are you able to revert this patch and test?
>>>>
>>>> -J
>>>>
>>>> ---
>>>> -Jay Vosburgh, jay.vosburgh@canonical.com
>>>>
>>>
>>>
>>>Just tested - yes, reverting that patch solves the issues.
>>
>> Chonggang,
>>
>> Reading the changelog in your commit referenced above, I'm not
>> entirely sure what actual problem it is fixing. Could you elaborate?
>>
>> As the patch appears to cause a regression, it needs to be
>> either fixed or reverted.
>>
>> Mahesh, you signed-off on it as well, perhaps you also have some
>> context?
>>
>
>I think the original idea behind it was to pass the LLDPDUs to the
>stack on the interface that they came on since this is considered to
>be link-local traffic and passing to bond-master would loose it's
>"linklocal-ness". This is true for LLDP and if you change the skb->dev
>of the packet, then you don't know which slave link it came on in
>(from LLDP consumer's perspective).
>
>I don't know much about STP but trunking two links and aggregating
>this link info through bond-master seems wrong. Just like LLDP, you
>are losing info specific to a link and the decision derived from that
>info could be wrong.
>
>Having said that, we determine "linklocal-ness" by looking at L2 and
>bondmaster shares this with lts slaves. So it does seem fair to pass
>those frames to the bonding-master but at the same time link-local
>traffic is supposed to be limited to the physical link (LLDP/STP/LACP
>etc). Your thoughts?
I agree the whole thing sounds kind of weird, but I'm curious as
to what Michal's actual use case is; he presumably has some practical
use for this, since he noticed that the behavior changed.
Michal, you mentioned MSTP and using 802.3ad (LACP) mode; how
does that combination work rationally given that the bond might send and
receive traffic across multiple slaves? Or does the switch side bundle
the ports together into a single logical interface for MSTP purposes?
On the TX side, I think the bond will likely balance all STP frames to
just one slave.
As for a resolution, presuming that Michal has some reasonable
use case, I'm thinking along the lines of reverting the new (leave frame
attached to slave) behavior for the general case and adding a special
case for LLDP and friends to get the new behavior. I'd like to avoid
adding any new options to bonding.
-J
---
-Jay Vosburgh, jay.vosburgh@canonical.com
^ permalink raw reply
* Re: [PATCH iproute2-next v2] net:sched: add action inheritdsfield to skbedit
From: Stephen Hemminger @ 2018-07-12 22:05 UTC (permalink / raw)
To: Qiaobin Fu
Cc: dsahern, davem, netdev, jhs, michel, marcelo.leitner,
xiyou.wangcong, dcaratti
In-Reply-To: <20180712160926.163317-1-qiaobinf@bu.edu>
On Thu, 12 Jul 2018 12:09:26 -0400
Qiaobin Fu <qiaobinf@bu.edu> wrote:
> + if (*flags & SKBEDIT_F_INHERITDSFIELD)
> + print_string(PRINT_ANY, "inheritdsfield", " %s",
> + "inheritdsfield");
Flags should be represented in JSON output as a null value (or boolean).
print_null(PRINT_ANY, "inheritdsfield", " %s", "inheritdsfield");
This will generate:
"inheritdsfield" : null,
Instead of:
"inheritdsfield" : "inheritdsfield",
^ permalink raw reply
* Re: [PATCH mlx5-next v1 2/8] net/mlx5: Add support for flow table destination number
From: Jason Gunthorpe @ 2018-07-12 22:05 UTC (permalink / raw)
To: Or Gerlitz
Cc: Yishai Hadas, Doug Ledford, Leon Romanovsky, RDMA mailing list,
Saeed Mahameed, linux-netdev
In-Reply-To: <CAJ3xEMg-cfRwZTwK2Eu2cdH1YuSF5rJy88YWYrbyWywKun5hPg@mail.gmail.com>
On Fri, Jul 13, 2018 at 12:51:10AM +0300, Or Gerlitz wrote:
> On Fri, Jul 13, 2018 at 12:26 AM, Jason Gunthorpe <jgg@mellanox.com> wrote:
> > On Fri, Jul 13, 2018 at 12:00:41AM +0300, Or Gerlitz wrote:
> >> On Wed, Jul 11, 2018 at 2:10 PM, Leon Romanovsky <leon@kernel.org> wrote:
> >> > From: Yishai Hadas <yishaih@mellanox.com>
> >> >
> >> > Add support to set a destination from a flow table number.
> >> > This functionality will be used in downstream patches from this
> >> > series by the DEVX stuff.
> >>
> >> Reading your cover letter, I still don't understand what is missing
> >> in the current mlx5 fs core API for your needs. After all, you do
> >> create flow tables from the IB driver through fs core calls, right?
> >> so @ the end of the day, you have the FT pointer to provide the
> >> core, why you need the FT number?
> >
> > Via the devx API userspace can create flow tables directly without
> > going to the driver's flow steering core.
>
> so why you change the core?
User space flow tables don't get any traffic until they are linked
into the main steering. The only ID the kernel gets for them when
adding this link is the actual PRM handle, not a pointer - hence the
change.
Jason
^ permalink raw reply
* Re: Re: [Qemu-devel] [PATCH v3 0/3] Use of unique identifier for pairing virtio and passthrough devices...
From: Siwei Liu @ 2018-07-12 22:19 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Cornelia Huck, si-wei liu, Roman Kagan, Venu Busireddy,
Marcel Apfelbaum, virtio-dev, qemu-devel, Samudrala, Sridhar,
Alexander Duyck, Netdev
In-Reply-To: <20180712235500-mutt-send-email-mst@kernel.org>
On Thu, Jul 12, 2018 at 2:00 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> On Thu, Jul 12, 2018 at 01:52:53PM -0700, Siwei Liu wrote:
>> The definition is incomplete due to lack of spec. There's no "host"
>> part defined yet in the host-guest interface. If match by MAC is an
>> interface, the same must be done on the host(device) side as well,
>> which has been agreed not the way to go. However, I don't think that's
>> what the author intends to do by interpreting his QEMU patch - it
>> missed the other parts as well, such as the feature negotiation and
>> how it interacts with the paired device.
>>
>> What I said is that match by MAC is just a guest implementation that
>> one can change at any time. We now have the group ID on QEMU, why
>> still sticking to matching by MAC? It shoulnd't be a host-guest
>> interface in the first place anyway.
>
> I think that match by MAC is a simple portable way to match devices.
> E.g. it will work seamlessly with niche things like zPCI. However
That's a good point. I'm not sure if it's a valid assumption that zPCI
should always use the same MAC address as that of virtio. Someone
who's more familiar with the use case may decide and work on that. It
means VFIO device has to take in the MAC address as an identifier to
the "-device vfio-pci,.." QEMU option. I think there's no point to
match device using group ID in QEMU while using MAC in the guest.
Based on that assumption, I'd go with making VIRTIO_NET_F_STANDBY to
match device based on group ID, while someone may come up with another
feature bit later, say VIRTIO_NET_F_STANDBY_BY_MAC when its QEMU
support is available. Would it make sense?
-Siwei
> there are other niche use-cases that aren't addressed by match by MAC
> such as PF pass-through as a primary, and the pci bridge trick addresses
> that at cost of some portability.
>
> So I see no issues supporting both mechanisms, but others on the TC
> might feel differently.
>
> --
> MST
^ permalink raw reply
* Re: [PATCH bpf] bpf: fix some bad __rcu annotations in bpf/core.c
From: Roman Gushchin @ 2018-07-12 22:26 UTC (permalink / raw)
To: Daniel Borkmann
Cc: netdev, linux-kernel, kernel-team, Alexei Starovoitov, yhs
In-Reply-To: <31c2b14a-8890-0ebd-bb59-01616e9d8d5d@iogearbox.net>
On Tue, Jul 10, 2018 at 10:03:19AM +0200, Daniel Borkmann wrote:
> Hi Roman,
>
> On 07/10/2018 03:59 AM, Roman Gushchin wrote:
> > Sparse shows some "incorrect type" warnings in the bpf core code.
>
> Thanks for taking a stab at these! It would really help if you could
> split the patch into a small series and fix each individual case that
> is problematic here.
>
> Please also add Fixes tags to the patches.
Sure.
The only problem which I have with these sparse warnings,
is that my cgroup local storage patchset touches some of
these lines, and I'm receiving automatic complains.
>
> More below.
>
> > They are caused by bad __rcu annotations:
> > 1) bpf_prog_array_alloc() returns an __rcu pointer, which isn't true.
> > At that moment it's obviously an exclusive "owning" pointer,
> > which is valid for an infinite amount of time, so __rcu is
> > meaningless.
> > 2) The progs local variable in compute_effective_progs should be
> > marked as __bpf too, it's a local variable, not shared with anyone
>
> Typo: __bpf ?
Yep, fixed.
>
> > else at all. The real __rcu variable is array pointer, which should
> > be assigned with rcu_assign_pointer.
> > 3) __rcu progs argument of bpf_prog_array_free() should be casted
> > to a simple pointer before calling kfree_rcu().
> > 4) There is a missing rcu_dereference() annotation in
> > bpf_prog_array_copy_to_user().
> > 5) old_array __rcu pointer in bpf_prog_array_copy() is used as
> > a "normal" non-__rcu pointer.
> >
> > These changes remove the following sparse warnings:
> > kernel/bpf/core.c:1544:31: warning: incorrect type in return expression (different address spaces)
> > kernel/bpf/core.c:1544:31: expected struct bpf_prog_array [noderef] <asn:4>*
> > kernel/bpf/core.c:1544:31: got void *
> > kernel/bpf/core.c:1548:17: warning: incorrect type in return expression (different address spaces)
> > kernel/bpf/core.c:1548:17: expected struct bpf_prog_array [noderef] <asn:4>*
> > kernel/bpf/core.c:1548:17: got struct bpf_prog_array *<noident>
> > kernel/bpf/core.c:1556:9: warning: incorrect type in argument 1 (different address spaces)
> > kernel/bpf/core.c:1556:9: expected struct callback_head *head
> > kernel/bpf/core.c:1556:9: got struct callback_head [noderef] <asn:4>*<noident>
> > kernel/bpf/core.c:1629:34: warning: incorrect type in initializer (different address spaces)
> > kernel/bpf/core.c:1629:34: expected struct bpf_prog **prog
> > kernel/bpf/core.c:1629:34: got struct bpf_prog *[noderef] <asn:4>*<noident>
> > kernel/bpf/core.c:1653:31: warning: incorrect type in assignment (different address spaces)
> > kernel/bpf/core.c:1653:31: expected struct bpf_prog **existing_prog
> > kernel/bpf/core.c:1653:31: got struct bpf_prog *[noderef] <asn:4>*<noident>
> > kernel/bpf/core.c:1681:15: warning: incorrect type in assignment (different address spaces)
> > kernel/bpf/core.c:1681:15: expected struct bpf_prog_array *array
> > kernel/bpf/core.c:1681:15: got struct bpf_prog_array [noderef] <asn:4>*
> > kernel/bpf/core.c:1687:31: warning: incorrect type in assignment (different address spaces)
> > kernel/bpf/core.c:1687:31: expected struct bpf_prog **[assigned] existing_prog
> > kernel/bpf/core.c:1687:31: got struct bpf_prog *[noderef] <asn:4>*<noident>
> >
> > Signed-off-by: Roman Gushchin <guro@fb.com>
> > Cc: Daniel Borkmann <daniel@iogearbox.net>
> > Cc: Alexei Starovoitov <ast@kernel.org>
> > ---
> > include/linux/bpf.h | 2 +-
> > kernel/bpf/cgroup.c | 7 +++----
> > kernel/bpf/core.c | 14 ++++++++------
> > 3 files changed, 12 insertions(+), 11 deletions(-)
> >
> > diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> > index 8827e797ff97..943fb08d8287 100644
> > --- a/include/linux/bpf.h
> > +++ b/include/linux/bpf.h
> > @@ -352,7 +352,7 @@ struct bpf_prog_array {
> > struct bpf_prog *progs[0];
> > };
> >
> > -struct bpf_prog_array __rcu *bpf_prog_array_alloc(u32 prog_cnt, gfp_t flags);
> > +struct bpf_prog_array *bpf_prog_array_alloc(u32 prog_cnt, gfp_t flags);
> > void bpf_prog_array_free(struct bpf_prog_array __rcu *progs);
> > int bpf_prog_array_length(struct bpf_prog_array __rcu *progs);
> > int bpf_prog_array_copy_to_user(struct bpf_prog_array __rcu *progs,
> > diff --git a/kernel/bpf/cgroup.c b/kernel/bpf/cgroup.c
> > index 3d83ee7df381..badabb0b435c 100644
> > --- a/kernel/bpf/cgroup.c
> > +++ b/kernel/bpf/cgroup.c
> > @@ -95,7 +95,7 @@ static int compute_effective_progs(struct cgroup *cgrp,
> > enum bpf_attach_type type,
> > struct bpf_prog_array __rcu **array)
> > {
> > - struct bpf_prog_array __rcu *progs;
> > + struct bpf_prog_array *progs;
> > struct bpf_prog_list *pl;
> > struct cgroup *p = cgrp;
> > int cnt = 0;
> > @@ -120,13 +120,12 @@ static int compute_effective_progs(struct cgroup *cgrp,
> > &p->bpf.progs[type], node) {
> > if (!pl->prog)
> > continue;
> > - rcu_dereference_protected(progs, 1)->
> > - progs[cnt++] = pl->prog;
> > + progs->progs[cnt++] = pl->prog;
> > }
> > p = cgroup_parent(p);
> > } while (p);
> >
> > - *array = progs;
> > + rcu_assign_pointer(*array, progs);
> > return 0;
> > }
> >
> > diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
> > index 1e5625d46414..f6e5b207a0d7 100644
> > --- a/kernel/bpf/core.c
> > +++ b/kernel/bpf/core.c
> > @@ -1538,7 +1538,7 @@ static struct {
> > .null_prog = NULL,
> > };
> >
> > -struct bpf_prog_array __rcu *bpf_prog_array_alloc(u32 prog_cnt, gfp_t flags)
> > +struct bpf_prog_array *bpf_prog_array_alloc(u32 prog_cnt, gfp_t flags)
> > {
> > if (prog_cnt)
> > return kzalloc(sizeof(struct bpf_prog_array) +
> > @@ -1550,10 +1550,11 @@ struct bpf_prog_array __rcu *bpf_prog_array_alloc(u32 prog_cnt, gfp_t flags)
>
> Looks good to me to here.
>
> > void bpf_prog_array_free(struct bpf_prog_array __rcu *progs)
> > {
> > - if (!progs ||
> > - progs == (struct bpf_prog_array __rcu *)&empty_prog_array.hdr)
> > + struct bpf_prog_array *array = rcu_access_pointer(progs);
>
> Can you elaborate on the rcu_access_pointer() part? This looks odd, at minimum
> this needs a comment explaining why it's needed. Is the __rcu annotation above
> even correct?
No, it's not. But fixing it causes to use rcu_access_pointer()
for almost every bpf_prog_array_delete_safe() call.
Still better probably, will go this path in v2.
>
> > +
> > + if (!array || array == &empty_prog_array.hdr)
> > return;
> > - kfree_rcu(progs, rcu);
> > + kfree_rcu(array, rcu);
> > }
> >
> > int bpf_prog_array_length(struct bpf_prog_array __rcu *progs)
> > @@ -1626,7 +1627,7 @@ int bpf_prog_array_copy_to_user(struct bpf_prog_array __rcu *progs,
> > void bpf_prog_array_delete_safe(struct bpf_prog_array __rcu *progs,
> > struct bpf_prog *old_prog)
> > {
> > - struct bpf_prog **prog = progs->progs;
> > + struct bpf_prog **prog = rcu_dereference(progs)->progs;
>
> Can you elaborate here as well? __rcu annotation buggy instead?
But here it's fine. Please, look at bpf_prog_array_length() and bpf_prog_array_copy_to_user().
Same applies here.
If we want to be more precise, the "progs" field in the bpf_prog_array should
also be marked with __rcu, but this is beyond the scope of this patch(set).
>
> > for (; *prog; prog++)
> > if (*prog == old_prog) {
> > @@ -1635,11 +1636,12 @@ void bpf_prog_array_delete_safe(struct bpf_prog_array __rcu *progs,
> > }
> > }
> >
> > -int bpf_prog_array_copy(struct bpf_prog_array __rcu *old_array,
> > +int bpf_prog_array_copy(struct bpf_prog_array __rcu *__old_array,
> > struct bpf_prog *exclude_prog,
> > struct bpf_prog *include_prog,
> > struct bpf_prog_array **new_array)
> > {
> > + struct bpf_prog_array *old_array = rcu_access_pointer(__old_array);
>
> Same comment here, this doesn't look right. We even fetch old_array->progs
> from it later on in this path.
Same here. old_array->progs field is not marked with __bpf currently.
Thanks!
^ permalink raw reply
* Re: [BUG] bonded interfaces drop bpdu (stp) frames
From: Michal Soltys @ 2018-07-12 22:14 UTC (permalink / raw)
To: Mahesh Bandewar (महेश बंडेवार),
Jay Vosburgh
Cc: Chonggang Li, linux-netdev
In-Reply-To: <CAF2d9jjXmiVZwDu-q+M5bmdrgyMr0U_tiUzxoCUiWwZT4NS5Cw@mail.gmail.com>
On 2018-07-12 23:26, Mahesh Bandewar (महेश बंडेवार) wrote:
> On Thu, Jul 12, 2018 at 11:03 AM, Jay Vosburgh
> <jay.vosburgh@canonical.com> wrote:
>> Michal Soltys <soltys@ziu.info> wrote:
>>
>>>On 07/12/2018 04:51 PM, Jay Vosburgh wrote:
>>>> Mahesh Bandewar (महेश बंडेवार) wrote:
>>>>
>>>>> On Wed, Jul 11, 2018 at 3:23 PM, Michal Soltys <soltys@ziu.info> wrote:
>>>>>>
>>>>>> Hi,
>>>>>>
>>>>>> As weird as that sounds, this is what I observed today after bumping
>>>>>> kernel version. I have a setup where 2 bonds are attached to linux
>>>>>> bridge and physically are connected to two switches doing MSTP (and
>>>>>> linux bridge is just passing them).
>>>>>>
>>>>>> Initially I suspected some changes related to bridge code - but quick
>>>>>> peek at the code showed nothing suspicious - and the part of it that
>>>>>> explicitly passes stp frames if stp is not enabled has seen little
>>>>>> changes (e.g. per-port group_fwd_mask added recently). Furthermore - if
>>>>>> regular non-bonded interfaces are attached everything works fine.
>>>>>>
>>>>>> Just to be sure I detached the bond (802.3ad mode) and checked it with
>>>>>> simple tcpdump (ether proto \\stp) - and indeed no hello packets were
>>>>>> there (with them being present just fine on active enslaved interface,
>>>>>> or on the bond device in earlier kernels).
>>>>>>
>>>>>> If time permits I'll bisect tommorow to pinpoint the commit, but from
>>>>>> quick todays test - 4.9.x is working fine, while 4.16.16 (tested on
>>>>>> debian) and 4.17.3 (tested on archlinux) are failing.
>>>>>>
>>>>>> Unless this is already a known issue (or you have any suggestions what
>>>>>> could be responsible).
>>>>>>
>>>>> I believe these are link-local-multicast messages and sometime back a
>>>>> change went into to not pass those frames to the bonding master. This
>>>>> could be the side effect of that.
>>>>
>>>> Mahesh, I suspect you're thinking of:
>>>>
>>>> commit b89f04c61efe3b7756434d693b9203cc0cce002e
>>>> Author: Chonggang Li <chonggangli@google.com>
>>>> Date: Sun Apr 16 12:02:18 2017 -0700
>>>>
>>>> bonding: deliver link-local packets with skb->dev set to link that packets arrived on
>>>>
>>>> Michal, are you able to revert this patch and test?
>>>>
>>>> -J
>>>>
>>>> ---
>>>> -Jay Vosburgh, jay.vosburgh@canonical.com
>>>>
>>>
>>>
>>>Just tested - yes, reverting that patch solves the issues.
>>
>> Chonggang,
>>
>> Reading the changelog in your commit referenced above, I'm not
>> entirely sure what actual problem it is fixing. Could you elaborate?
>>
>> As the patch appears to cause a regression, it needs to be
>> either fixed or reverted.
>>
>> Mahesh, you signed-off on it as well, perhaps you also have some
>> context?
>>
>
> I think the original idea behind it was to pass the LLDPDUs to the
> stack on the interface that they came on since this is considered to
> be link-local traffic and passing to bond-master would loose it's
> "linklocal-ness". This is true for LLDP and if you change the skb->dev
> of the packet, then you don't know which slave link it came on in
> (from LLDP consumer's perspective).
>
> I don't know much about STP but trunking two links and aggregating
> this link info through bond-master seems wrong. Just like LLDP, you
> are losing info specific to a link and the decision derived from that
> info could be wrong.
>
> Having said that, we determine "linklocal-ness" by looking at L2 and
> bondmaster shares this with lts slaves. So it does seem fair to pass
> those frames to the bonding-master but at the same time link-local
> traffic is supposed to be limited to the physical link (LLDP/STP/LACP
> etc). Your thoughts?
>
But, isn't bond de-facto considered the "physical link" ? Not directly
of course, but say an LLDP daemon would likely be more interested in
getting LLDP data from a bond device (or a bridge device, if the bond is
attached to one), than from its enslaved interfaces (and enslaved
interfaces can be changed, not mentioning potentially complex setup
itself, even if usually it's just lacp&go ).
ITOW, blocking link-local multicasts on bond level (among those - bpdu,
pae, lldp) is a bit like if the interface itself hid LACP before bond code.
A few other examples:
- putting bonds in a bridge is pretty normal thing - and whether the
bridge interpretes the spanning tree data itself (via in-kernel classic
stp or userspace daemon for e.g. rstp) or passes the trafic, it must see
the BPDU frames. Otherwise it becomes blind to the whole spanning tree
protocol - and implicitly other switches around - real or virtual ones.
It's literally instant loop disaster. br_input.c specifically takes care
to pass those frames if the bridge has stp turned off
- "group_fwd_mask" (again in bridge context) has been added to bridge
code - and recently as a per-port knob as well - to specifically allow
the control of what kind of "link-local" stuff is passed or not. LLDP
and 802.1X PAE were, afaik, the main reasons for that sysfs variable.
The per-port setting is even more relaxed (iirc, only pause frames are
not passable)
- LLDP daemon example - as above
^ permalink raw reply
* Re: [PATCH net] net: Don't copy pfmemalloc flag in __copy_skb_header()
From: David Miller @ 2018-07-12 22:15 UTC (permalink / raw)
To: sbrivio; +Cc: mgorman, eric.dumazet, fw, ptalbert, netdev
In-Reply-To: <93db92329a9964c336965c166e5c858cf46cd0a5.1531305883.git.sbrivio@redhat.com>
From: Stefano Brivio <sbrivio@redhat.com>
Date: Wed, 11 Jul 2018 14:39:42 +0200
> The pfmemalloc flag indicates that the skb was allocated from
> the PFMEMALLOC reserves, and the flag is currently copied on skb
> copy and clone.
>
> However, an skb copied from an skb flagged with pfmemalloc
> wasn't necessarily allocated from PFMEMALLOC reserves, and on
> the other hand an skb allocated that way might be copied from an
> skb that wasn't.
>
> So we should not copy the flag on skb copy, and rather decide
> whether to allow an skb to be associated with sockets unrelated
> to page reclaim depending only on how it was allocated.
>
> Move the pfmemalloc flag before headers_start[0] using an
> existing 1-bit hole, so that __copy_skb_header() doesn't copy
> it.
>
> When cloning, we'll now take care of this flag explicitly,
> contravening to the warning comment of __skb_clone().
>
> While at it, restore the newline usage introduced by commit
> b19372273164 ("net: reorganize sk_buff for faster
> __copy_skb_header()") to visually separate bytes used in
> bitfields after headers_start[0], that was gone after commit
> a9e419dc7be6 ("netfilter: merge ctinfo into nfct pointer storage
> area"), and describe the pfmemalloc flag in the kernel-doc
> structure comment.
>
> This doesn't change the size of sk_buff or cacheline boundaries,
> but consolidates the 15 bits hole before tc_index into a 2 bytes
> hole before csum, that could now be filled more easily.
>
> Reported-by: Patrick Talbert <ptalbert@redhat.com>
> Fixes: c93bdd0e03e8 ("netvm: allow skb allocation to use PFMEMALLOC reserves")
> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
Applied and queued up for -stable, thank you.
^ permalink raw reply
* Re: [PATCH v2 net-next 1/9] lan743x: Add support for ethtool get_drvinfo
From: Andrew Lunn @ 2018-07-12 22:28 UTC (permalink / raw)
To: Bryan Whitehead; +Cc: davem, netdev, UNGLinuxDriver, richardcochran
In-Reply-To: <1531422306-6547-2-git-send-email-Bryan.Whitehead@microchip.com>
On Thu, Jul 12, 2018 at 03:04:58PM -0400, Bryan Whitehead wrote:
> Implement ethtool get_drvinfo
>
> Signed-off-by: Bryan Whitehead <Bryan.Whitehead@microchip.com>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Andrew
^ permalink raw reply
* Re: [PATCH v2 net-next 2/9] lan743x: Add support for ethtool link settings
From: Andrew Lunn @ 2018-07-12 22:29 UTC (permalink / raw)
To: Bryan Whitehead; +Cc: davem, netdev, UNGLinuxDriver, richardcochran
In-Reply-To: <1531422306-6547-3-git-send-email-Bryan.Whitehead@microchip.com>
On Thu, Jul 12, 2018 at 03:04:59PM -0400, Bryan Whitehead wrote:
> Use default link setting functions
>
> Signed-off-by: Bryan Whitehead <Bryan.Whitehead@microchip.com>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Andrew
^ permalink raw reply
* Re: [PATCH v2 net-next 3/9] lan743x: Add support for ethtool statistics
From: Andrew Lunn @ 2018-07-12 22:31 UTC (permalink / raw)
To: Bryan Whitehead; +Cc: davem, netdev, UNGLinuxDriver, richardcochran
In-Reply-To: <1531422306-6547-4-git-send-email-Bryan.Whitehead@microchip.com>
On Thu, Jul 12, 2018 at 03:05:00PM -0400, Bryan Whitehead wrote:
> Implement ethtool statistics
>
> Signed-off-by: Bryan Whitehead <Bryan.Whitehead@microchip.com>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Andrew
^ permalink raw reply
* Re: [PATCH v2 net-next 4/9] lan743x: Add support for ethtool message level
From: Andrew Lunn @ 2018-07-12 22:31 UTC (permalink / raw)
To: Bryan Whitehead; +Cc: davem, netdev, UNGLinuxDriver, richardcochran
In-Reply-To: <1531422306-6547-5-git-send-email-Bryan.Whitehead@microchip.com>
On Thu, Jul 12, 2018 at 03:05:01PM -0400, Bryan Whitehead wrote:
> Implement ethtool message level
>
> Signed-off-by: Bryan Whitehead <Bryan.Whitehead@microchip.com>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Andrew
^ permalink raw reply
* Re: [PATCH v2 net-next 5/9] lan743x: Add support for ethtool eeprom access
From: Andrew Lunn @ 2018-07-12 22:35 UTC (permalink / raw)
To: Bryan Whitehead; +Cc: davem, netdev, UNGLinuxDriver, richardcochran
In-Reply-To: <1531422306-6547-6-git-send-email-Bryan.Whitehead@microchip.com>
On Thu, Jul 12, 2018 at 03:05:02PM -0400, Bryan Whitehead wrote:
> Implement ethtool eeprom access
> Also provides access to OTP (One Time Programming)
>
> Signed-off-by: Bryan Whitehead <Bryan.Whitehead@microchip.com>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Andrew
^ permalink raw reply
* Re: [PATCH v2 net-next 6/9] lan743x: Add power management support
From: Andrew Lunn @ 2018-07-12 22:42 UTC (permalink / raw)
To: Bryan Whitehead; +Cc: davem, netdev, UNGLinuxDriver, richardcochran
In-Reply-To: <1531422306-6547-7-git-send-email-Bryan.Whitehead@microchip.com>
> +#ifdef CONFIG_PM
> +static void lan743x_ethtool_get_wol(struct net_device *netdev,
> + struct ethtool_wolinfo *wol)
> +{
> + struct lan743x_adapter *adapter = netdev_priv(netdev);
> +
> + wol->supported = WAKE_BCAST | WAKE_UCAST | WAKE_MCAST |
> + WAKE_MAGIC | WAKE_PHY | WAKE_ARP;
> +
> + wol->wolopts = adapter->wolopts;
> +}
> +#endif /* CONFIG_PM */
> +
> +#ifdef CONFIG_PM
> +static int lan743x_ethtool_set_wol(struct net_device *netdev,
> + struct ethtool_wolinfo *wol)
> +{
> + struct lan743x_adapter *adapter = netdev_priv(netdev);
> +
> + if (wol->wolopts & WAKE_MAGICSECURE)
> + return -EOPNOTSUPP;
> +
> + adapter->wolopts = 0;
> + if (wol->wolopts & WAKE_UCAST)
> + adapter->wolopts |= WAKE_UCAST;
> + if (wol->wolopts & WAKE_MCAST)
> + adapter->wolopts |= WAKE_MCAST;
> + if (wol->wolopts & WAKE_BCAST)
> + adapter->wolopts |= WAKE_BCAST;
> + if (wol->wolopts & WAKE_MAGIC)
> + adapter->wolopts |= WAKE_MAGIC;
> + if (wol->wolopts & WAKE_PHY)
> + adapter->wolopts |= WAKE_PHY;
> + if (wol->wolopts & WAKE_ARP)
> + adapter->wolopts |= WAKE_ARP;
> +
> + device_set_wakeup_enable(&adapter->pdev->dev, (bool)wol->wolopts);
> +
> + phy_ethtool_set_wol(netdev->phydev, wol);
Hi Bryan
This seems asymmetric. set_wol you call into the phylib to enable wol
in the PHY. But get_wol does not call into phylib. So the phy has no
chance to set what it supports.
Andrew
^ permalink raw reply
* Re: [PATCH v2 net-next 7/9] lan743x: Add EEE support
From: Andrew Lunn @ 2018-07-12 22:49 UTC (permalink / raw)
To: Bryan Whitehead; +Cc: davem, netdev, UNGLinuxDriver, richardcochran
In-Reply-To: <1531422306-6547-8-git-send-email-Bryan.Whitehead@microchip.com>
> +static int lan743x_ethtool_set_eee(struct net_device *netdev,
> + struct ethtool_eee *eee)
> +{
> + struct lan743x_adapter *adapter = netdev_priv(netdev);
> + struct phy_device *phydev = NULL;
> + u32 buf = 0;
> + int ret = 0;
> +
> + if (!netdev)
> + return -EINVAL;
> + adapter = netdev_priv(netdev);
> + if (!adapter)
> + return -EINVAL;
> + phydev = netdev->phydev;
> + if (!phydev)
> + return -EIO;
> + if (!phydev->drv) {
> + netif_err(adapter, drv, adapter->netdev,
> + "Missing PHY Driver\n");
> + return -EIO;
> + }
> +
> + if (eee->eee_enabled) {
> + ret = phy_init_eee(phydev, 0);
> + if (ret) {
> + netif_err(adapter, drv, adapter->netdev,
> + "EEE initialization failed\n");
> + return ret;
> + }
> +
> + buf = lan743x_csr_read(adapter, MAC_CR);
> + buf |= MAC_CR_EEE_EN_;
> + lan743x_csr_write(adapter, MAC_CR, buf);
> +
> + phy_ethtool_set_eee(phydev, eee);
> +
> + buf = (u32)eee->tx_lpi_timer;
> + lan743x_csr_write(adapter, MAC_EEE_TX_LPI_REQ_DLY_CNT, buf);
> + netif_info(adapter, drv, adapter->netdev, "Enabled EEE\n");
> + } else {
> + buf = lan743x_csr_read(adapter, MAC_CR);
> + buf &= ~MAC_CR_EEE_EN_;
> + lan743x_csr_write(adapter, MAC_CR, buf);
> + netif_info(adapter, drv, adapter->netdev, "Disabled EEE\n");
> + }
> +
Hi Bryan
You should call phy_ethtool_set_eee() in both cases, so that it gets
disabled in the PHY as well. It needs to stop advertising it.
Andrew
^ permalink raw reply
* Proposal
From: Miss Victoria Mehmet @ 2018-07-12 22:46 UTC (permalink / raw)
To: Recipients
Hello
I have a business proposal of mutual benefits i would like to discuss with
you.
^ permalink raw reply
* Re: [BUG] bonded interfaces drop bpdu (stp) frames
From: Michal Soltys @ 2018-07-12 23:14 UTC (permalink / raw)
To: Jay Vosburgh,
Mahesh Bandewar (महेश बंडेवार)
Cc: Chonggang Li, linux-netdev
In-Reply-To: <30874.1531433032@nyx>
On 2018-07-13 00:03, Jay Vosburgh wrote:
> Mahesh Bandewar (महेश बंडेवार) wrote:
>
>>On Thu, Jul 12, 2018 at 11:03 AM, Jay Vosburgh
>><jay.vosburgh@canonical.com> wrote:
>>> Michal Soltys <soltys@ziu.info> wrote:
>>>
>>>>On 07/12/2018 04:51 PM, Jay Vosburgh wrote:
>>>>> Mahesh Bandewar (महेश बंडेवार) wrote:
>>>>>
>>>>>> On Wed, Jul 11, 2018 at 3:23 PM, Michal Soltys <soltys@ziu.info> wrote:
>>>>>>>
>>>>>>> Hi,
>>>>>>>
>>>>>>> As weird as that sounds, this is what I observed today after bumping
>>>>>>> kernel version. I have a setup where 2 bonds are attached to linux
>>>>>>> bridge and physically are connected to two switches doing MSTP (and
>>>>>>> linux bridge is just passing them).
>>>>>>>
>>>>>>> Initially I suspected some changes related to bridge code - but quick
>>>>>>> peek at the code showed nothing suspicious - and the part of it that
>>>>>>> explicitly passes stp frames if stp is not enabled has seen little
>>>>>>> changes (e.g. per-port group_fwd_mask added recently). Furthermore - if
>>>>>>> regular non-bonded interfaces are attached everything works fine.
>>>>>>>
>>>>>>> Just to be sure I detached the bond (802.3ad mode) and checked it with
>>>>>>> simple tcpdump (ether proto \\stp) - and indeed no hello packets were
>>>>>>> there (with them being present just fine on active enslaved interface,
>>>>>>> or on the bond device in earlier kernels).
>>>>>>>
>>>>>>> If time permits I'll bisect tommorow to pinpoint the commit, but from
>>>>>>> quick todays test - 4.9.x is working fine, while 4.16.16 (tested on
>>>>>>> debian) and 4.17.3 (tested on archlinux) are failing.
>>>>>>>
>>>>>>> Unless this is already a known issue (or you have any suggestions what
>>>>>>> could be responsible).
>>>>>>>
>>>>>> I believe these are link-local-multicast messages and sometime back a
>>>>>> change went into to not pass those frames to the bonding master. This
>>>>>> could be the side effect of that.
>>>>>
>>>>> Mahesh, I suspect you're thinking of:
>>>>>
>>>>> commit b89f04c61efe3b7756434d693b9203cc0cce002e
>>>>> Author: Chonggang Li <chonggangli@google.com>
>>>>> Date: Sun Apr 16 12:02:18 2017 -0700
>>>>>
>>>>> bonding: deliver link-local packets with skb->dev set to link that packets arrived on
>>>>>
>>>>> Michal, are you able to revert this patch and test?
>>>>>
>>>>> -J
>>>>>
>>>>> ---
>>>>> -Jay Vosburgh, jay.vosburgh@canonical.com
>>>>>
>>>>
>>>>
>>>>Just tested - yes, reverting that patch solves the issues.
>>>
>>> Chonggang,
>>>
>>> Reading the changelog in your commit referenced above, I'm not
>>> entirely sure what actual problem it is fixing. Could you elaborate?
>>>
>>> As the patch appears to cause a regression, it needs to be
>>> either fixed or reverted.
>>>
>>> Mahesh, you signed-off on it as well, perhaps you also have some
>>> context?
>>>
>>
>>I think the original idea behind it was to pass the LLDPDUs to the
>>stack on the interface that they came on since this is considered to
>>be link-local traffic and passing to bond-master would loose it's
>>"linklocal-ness". This is true for LLDP and if you change the skb->dev
>>of the packet, then you don't know which slave link it came on in
>>(from LLDP consumer's perspective).
>>
>>I don't know much about STP but trunking two links and aggregating
>>this link info through bond-master seems wrong. Just like LLDP, you
>>are losing info specific to a link and the decision derived from that
>>info could be wrong.
>>
>>Having said that, we determine "linklocal-ness" by looking at L2 and
>>bondmaster shares this with lts slaves. So it does seem fair to pass
>>those frames to the bonding-master but at the same time link-local
>>traffic is supposed to be limited to the physical link (LLDP/STP/LACP
>>etc). Your thoughts?
>
> I agree the whole thing sounds kind of weird, but I'm curious as
> to what Michal's actual use case is; he presumably has some practical
> use for this, since he noticed that the behavior changed.
>
The whole "link-local" term is a bit I don't know - at this point it
feels like too many things were thrown into single bag and it got
somewhat confusing (bpdu, lldp, pause frames, lacp, pae, qinq mulitcast
that afaik has its own address) - I added some examples in another reply
I did at the same time as you were typing this one =)
> Michal, you mentioned MSTP and using 802.3ad (LACP) mode; how
> does that combination work rationally given that the bond might send and
> receive traffic across multiple slaves? Or does the switch side bundle
> the ports together into a single logical interface for MSTP purposes?
> On the TX side, I think the bond will likely balance all STP frames to
> just one slave.
>
The basic concept - two "main" switches with "important" machines
connected to those. One switch dies and everything keeps working. With
no unused ports and so on.
In more details:
Originally I was trying MSTP daemon (on "important" machines) which
seems quite well and completely coded, but cannot really work correctly
- as afaik you can't put port (in linux bridge conext) in different
forwarding/blocking/etc. state per-region - itow per group of vlans (or
mstpd didn't know how to do that, or it wasn't implemented - I didn't
look too deep back then, though my interest resurfaced in recent days).
So that option was out of the question. But any switch, real or not,
/must/ pass bpdu frames if it doesn't interpret them. So instead of
having active mstp participant, we have passive linux bridge that passes
the frames and the two real switches around that care of mstp, treating
the linux as a shared segment. The costs/priorities/etc. on the real
swtiches are set so one bond handles two regions, and the other bond
handles other two regions. If any of the real switches dies or is taken
down for e.g. firmware update - the bond going to the other switch
handles all four regions (failover is of course not as fast as with
active rstp/mstp participation, but works quite well none the less -
around 10s after some tuning).
We could have used RSTP for that purpose as well - but that being all or
nothing in context of per-port blocking/forwarding, would leave half of
the ports unused - and we wanted to avoid that (that's why MSTP was
created after all).
Instead of using 2 bonds (2 interfaces each) we could just use 4
interfaces directly, one per region. But two of those regions see very
little traffic, so we put more and less active regions in pairs.
> As for a resolution, presuming that Michal has some reasonable
> use case, I'm thinking along the lines of reverting the new (leave frame
> attached to slave) behavior for the general case and adding a special
> case for LLDP and friends to get the new behavior. I'd like to avoid
> adding any new options to bonding.
>
My use case aside, this will cause issues for anyone attaching bond
(instead of direct interface or veth) to a bridge and doing something
more complex with it - whether related to stp or to selectively passing
e.g. lldp using group_fwd_mask sysfs. Or having LLDP daemon (e.g.
systemd-resolvd to not look far away) told to do LLDP on bond device
(even most basic active-backup case) and remaining blind. Or anything
else that expects to see/pass those multicasts on/via bonded device
(which is just a convenient way to create virtual interface out of real
interfaces after all - ITOW shouldn't probably make any calls in this
regard).
^ permalink raw reply
* Re: [net-next PATCH] net: ipv4: fix listify ip_rcv_finish in case of forwarding
From: David Miller @ 2018-07-12 23:41 UTC (permalink / raw)
To: brouer; +Cc: ecree, netdev
In-Reply-To: <153132125549.13161.16380200872856218805.stgit@firesoul>
From: Jesper Dangaard Brouer <brouer@redhat.com>
Date: Wed, 11 Jul 2018 17:01:20 +0200
> In commit 5fa12739a53d ("net: ipv4: listify ip_rcv_finish") calling
> dst_input(skb) was split-out. The ip_sublist_rcv_finish() just calls
> dst_input(skb) in a loop.
>
> The problem is that ip_sublist_rcv_finish() forgot to remove the SKB
> from the list before invoking dst_input(). Further more we need to
> clear skb->next as other parts of the network stack use another kind
> of SKB lists for xmit_more (see dev_hard_start_xmit).
>
> A crash occurs if e.g. dst_input() invoke ip_forward(), which calls
> dst_output()/ip_output() that eventually calls __dev_queue_xmit() +
> sch_direct_xmit(), and a crash occurs in validate_xmit_skb_list().
>
> This patch only fixes the crash, but there is a huge potential for
> a performance boost if we can pass an SKB-list through to ip_forward.
>
> Fixes: 5fa12739a53d ("net: ipv4: listify ip_rcv_finish")
> Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com>
> ---
> Only driver sfc actually uses this, but I don't have this NIC, so I
> tested this on mlx5, with my own changes to make it use netif_receive_skb_list(),
> but I'm not ready to upstream the mlx5 driver change yet.
Applied, thanks Jesper.
This whole:
list_del();
skb->next = NULL;
business is exactly the kind of dragons I was worried about when starting
to use list_head with SKBs.
There is a similar fix wrt. the GRO stuff that I'm about to apply as well.
It definitely is better if we don't have to forcefully hand off NULL
->next next pointers like this in the long term.
^ permalink raw reply
* Re: [PATCH net-next 00/10] s390/qeth: updates 2018-07-11
From: David Miller @ 2018-07-12 23:43 UTC (permalink / raw)
To: jwi; +Cc: netdev, linux-s390, schwidefsky, heiko.carstens, raspl, ubraun
In-Reply-To: <20180711154247.5523-1-jwi@linux.ibm.com>
From: Julian Wiedmann <jwi@linux.ibm.com>
Date: Wed, 11 Jul 2018 17:42:37 +0200
> please apply this first batch of qeth patches for net-next. It brings the
> usual cleanups, and some performance improvements to the transmit paths.
Series applied, thank you.
^ permalink raw reply
* [net-next, 2/3] tcp: convert icsk_user_timeout from jiffies to msecs
From: Jon Maxwell @ 2018-07-13 0:02 UTC (permalink / raw)
To: davem
Cc: edumazet, ncardwell, David.Laight, kuznet, yoshfuji, netdev,
linux-kernel, jmaxwell
Create a seperate helper routine called tcp_retransmit_stamp() as per Neal
Cardwells suggestion. To be used by the final commit in this series and
retransmits_timed_out().
Signed-off-by: Jon Maxwell <jmaxwell37@gmail.com>
---
net/ipv4/tcp_timer.c | 25 +++++++++++++++++--------
1 file changed, 17 insertions(+), 8 deletions(-)
diff --git a/net/ipv4/tcp_timer.c b/net/ipv4/tcp_timer.c
index fa34984d0b12..8ab8c9645294 100644
--- a/net/ipv4/tcp_timer.c
+++ b/net/ipv4/tcp_timer.c
@@ -22,6 +22,20 @@
#include <linux/gfp.h>
#include <net/tcp.h>
+u32 tcp_retransmit_stamp(const struct sock *sk)
+{
+ u32 start_ts = tcp_sk(sk)->retrans_stamp;
+
+ if (unlikely(!start_ts)) {
+ struct sk_buff *head = tcp_rtx_queue_head(sk);
+
+ if (!head)
+ return 0;
+ start_ts = tcp_skb_timestamp(head);
+ }
+ return start_ts;
+}
+
/**
* tcp_write_err() - close socket and save error info
* @sk: The socket the error has appeared on.
@@ -166,14 +180,9 @@ static bool retransmits_timed_out(struct sock *sk,
if (!inet_csk(sk)->icsk_retransmits)
return false;
- start_ts = tcp_sk(sk)->retrans_stamp;
- if (unlikely(!start_ts)) {
- struct sk_buff *head = tcp_rtx_queue_head(sk);
-
- if (!head)
- return false;
- start_ts = tcp_skb_timestamp(head);
- }
+ start_ts = tcp_retransmit_stamp(sk);
+ if (!start_ts)
+ return false;
if (likely(timeout == 0)) {
linear_backoff_thresh = ilog2(TCP_RTO_MAX/rto_base);
--
2.13.6
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox