* Re: [PATCH bpf-next v2] bpf: bpftool, fix documentation for attach types
From: Daniel Borkmann @ 2019-02-19 16:24 UTC (permalink / raw)
To: Alban Crequy, ast, quentin.monnet
Cc: netdev, linux-kernel, john.fastabend, alban, iago
In-Reply-To: <20190219141332.23103-1-alban@kinvolk.io>
On 02/19/2019 03:13 PM, Alban Crequy wrote:
> From: Alban Crequy <alban@kinvolk.io>
>
> bpftool has support for attach types "stream_verdict" and
> "stream_parser" but the documentation was referring to them as
> "skb_verdict" and "skb_parse". The inconsistency comes from commit
> b7d3826c2ed6 ("bpf: bpftool, add support for attaching programs to
> maps").
>
> This patch changes the documentation to match the implementation:
> - "bpftool prog help"
> - man pages
> - bash completion
>
> Signed-off-by: Alban Crequy <alban@kinvolk.io>
Applied, thanks Alban!
^ permalink raw reply
* Re: [PATCH net-next v2 2/3] net: dsa: mv88e6xxx: add support for bridge flags
From: Vivien Didelot @ 2019-02-19 17:00 UTC (permalink / raw)
To: Russell King - ARM Linux admin
Cc: Andrew Lunn, Florian Fainelli, Heiner Kallweit, David S. Miller,
netdev
In-Reply-To: <20190219162435.f5zl5harbarwy6bj@shell.armlinux.org.uk>
Hi Russell,
On Tue, 19 Feb 2019 16:24:35 +0000, Russell King - ARM Linux admin <linux@armlinux.org.uk> wrote:
> > > +static unsigned long mv88e6xxx_bridge_flags_support(struct dsa_switch *ds)
> > > +{
> > > + struct mv88e6xxx_chip *chip = ds->priv;
> > > + unsigned long support = 0;
> > > +
> > > + if (chip->info->ops->port_set_egress_floods)
> > > + support |= BR_FLOOD | BR_MCAST_FLOOD;
> > > +
> > > + return support;
> > > +}
> >
> > I think that it isn't necessary to propagate the notion of bridge flags down
> > to the DSA drivers. It might be just enough to add:
> >
> > port_egress_flood(dsa_switch *ds, int port, bool uc, bool mc)
> >
> > to dsa_switch_ops and set BR_FLOOD | BR_MCAST_FLOOD from the DSA core,
> > if the targeted driver has ds->ops->port_set_egress_flood. What do you think?
>
> There are two other flags that I haven't covered which the bridge code
> expects to be offloaded, and those are the broadcast flood flag and
> the learning flag.
I see. What does the bridge code do if these flags are set? Does it expect
the underlying devices to handle ff:ff:ff:ff:ff:ff magically or does it
program this entry into the bridged ports?
In the latter case we have almost nothing to do. In the former case, we can
make the core call dsa_port_mdb_add on setup and when a VLAN is added.
mv88e6xxx tries to be smart and is already doing that and I'm really not a fan.
If tomorrow there's a switch capable of simply toggling a bit to do that,
we can add a new ops and skip the port_mdb_add call in the core.
> I know that the Marvell switches don't have a bit to control the
> broadcast flooding, that appears to be controlled via a static entry
> in the ATU which would have to be modified as the broadcast flood flag
> is manipulated. I don't know how that is handled in other bridges.
>
> Do we want to include the broadcast flood in the above prototype?
> If we go for this, how do we detect which options a switch supports?
If the necessary dsa_switch_ops routine is correctly prototyped, having it
implemented by a driver or not should be enough to inform the core that the
related feature(s) is/are supported by the switch.
I'll try to give a bit more context on why I'd prefer this approach, hoping
it makes sense: a switch driver does not need to understand bridge flags
per-se, the core should give enough abstraction to this layer (and any other
net-specifics). The core just needs to know if a driver can program this or
that. More importantly, it can easily become messy to maintain switch-cases
of arbitrary flags in all drivers and the core.
Thanks,
Vivien
^ permalink raw reply
* Re: [PATCH net-next v2 2/3] net: dsa: mv88e6xxx: add support for bridge flags
From: Russell King - ARM Linux admin @ 2019-02-19 17:00 UTC (permalink / raw)
To: Vivien Didelot
Cc: Andrew Lunn, Florian Fainelli, Heiner Kallweit, David S. Miller,
netdev
In-Reply-To: <20190219111612.GF27578@t480s.localdomain>
On Tue, Feb 19, 2019 at 11:16:12AM -0500, Vivien Didelot wrote:
> Hi Russell,
>
> On Sun, 17 Feb 2019 16:32:34 +0000, Russell King <rmk+kernel@armlinux.org.uk> wrote:
> > +static int mv88e6xxx_port_bridge_flags(struct dsa_switch *ds, int port,
> > + unsigned long flags)
> > +{
> > + struct mv88e6xxx_chip *chip = ds->priv;
> > + bool unicast, multicast;
> > + int ret = -EOPNOTSUPP;
> > +
> > + unicast = dsa_is_cpu_port(ds, port) || dsa_is_dsa_port(ds, port) ||
> > + flags & BR_FLOOD;
> > + multicast = flags & BR_MCAST_FLOOD;
> > +
> > + mutex_lock(&chip->reg_lock);
> > + if (chip->info->ops->port_set_egress_floods)
> > + ret = chip->info->ops->port_set_egress_floods(chip, port,
> > + unicast,
> > + multicast);
> > + mutex_unlock(&chip->reg_lock);
> > +
> > + return ret;
> > +}
> > +
> > +static unsigned long mv88e6xxx_bridge_flags_support(struct dsa_switch *ds)
> > +{
> > + struct mv88e6xxx_chip *chip = ds->priv;
> > + unsigned long support = 0;
> > +
> > + if (chip->info->ops->port_set_egress_floods)
> > + support |= BR_FLOOD | BR_MCAST_FLOOD;
> > +
> > + return support;
> > +}
>
> I think that it isn't necessary to propagate the notion of bridge flags down
> to the DSA drivers. It might be just enough to add:
>
> port_egress_flood(dsa_switch *ds, int port, bool uc, bool mc)
>
> to dsa_switch_ops and set BR_FLOOD | BR_MCAST_FLOOD from the DSA core,
> if the targeted driver has ds->ops->port_set_egress_flood. What do you think?
I've just changed my last patch to set these modes from
dsa_port_bridge_join() and dsa_port_bridge_leave(), and while testing,
I notice this on the ZII rev B board:
At boot (without anything connected to any of the switch ports):
br0: port 1(lan0) entered blocking state
br0: port 1(lan0) entered disabled state
device lan0 entered promiscuous mode
device eth1 entered promiscuous mode
br0: port 2(lan1) entered blocking state
br0: port 2(lan1) entered disabled state
device lan1 entered promiscuous mode
...
I then removed lan0 from the bridge:
device lan0 left promiscuous mode
br0: port 1(lan0) entered disabled state
and then added it back:
br0: port 1(lan0) entered blocking state
br0: port 1(lan0) entered disabled state
device lan0 entered promiscuous mode
Now, you'd expect lan0 and lan1 to be configured the same at this
point, and the same as it was before lan0 was removed from the bridge?
lan0 is port 0, lan1 is port 1 on this switch - and the register debug
says:
GLOBAL GLOBAL2 SERDES 0 1 2 3 4 5 6
0: c800 0 1140 500f 500f 500f 500f 500f 4e07 4d04
...
4: 40a8 258 1e0 43c 43d 43d 7c 430 53f 373f
Note that port 0 is in disabled state, but port 1 and 2 are in
blocking state... but wait, the kernel printed a message saying it was
in disabled state!
If I do the same for lan1, port 1 above changed from 0x43d to 0x433 as
expected, and then returns to 0x43c.
It looks like DSA isn't always in sync with bridge as per port state.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up
^ permalink raw reply
* Re: Three questions about busy poll
From: Willem de Bruijn @ 2019-02-19 17:04 UTC (permalink / raw)
To: Cong Wang
Cc: Alexander Duyck, Eric Dumazet, Samudrala, Sridhar,
Linux Kernel Network Developers
In-Reply-To: <CAM_iQpWtg6=qjQ=JFQyEONHujHX0BWEswejtc==RxocHj365uQ@mail.gmail.com>
On Mon, Feb 18, 2019 at 11:30 PM Cong Wang <xiyou.wangcong@gmail.com> wrote:
>
> On Fri, Feb 15, 2019 at 2:47 PM Willem de Bruijn
> <willemdebruijn.kernel@gmail.com> wrote:
> >
> > > > > 2. Why there is no socket option for sysctl.net.busy_poll? Clearly
> > > > > sysctl_net_busy_poll is global and SO_BUSY_POLL only works for
> > > > > sysctl.net.busy_read.
> > > >
> > > > I guess because of how sock_poll works. In that case it is not needed.
> > > > The poll duration applies more to the pollset than any of the
> > > > individual sockets, too.
> > >
> > >
> > > Good point, it's probably like struct eventpoll vs. struct epitem.
> > >
> > > The reason why I am looking for a per-socket tuning is to minimize
> > > the impact of setting busy_poll. I don't know if it is possible to somehow
> > > make this per-socket via epoll interfaces, perhaps fundamentally
> > > it is impossible?
> >
> > I think it may be possible. The way busy_read and busy_poll work
> > in sock_poll is that the sum of all (per socket tunable) busy_read
> > durations on the sockets in the pollset is ~bound by (global) busy_poll.
>
>
> Good idea!!
>
> I was actually thinking about checking sk->sk_ll_usec
> ep_set_busy_poll_napi_id(). Your idea sounds better than mine.
> Maybe we can do both together. :)
>
>
> >
> > The epoll implementation is restricted in the sense that it polls only
> > on one napi_id at a time. Alongside setting ep->napi_id in
> > ep_set_busy_poll_napi_id, we could also set a new ep field takes
> > the min of the global busy_poll and sk->sk_ll_usec.
> >
> > Though I guess you want to be able to poll on a given pollset
> > without setting the global sysctl_net_busy_poll at all? That
> > would be a useful feature both for epoll and poll/select. But
> > definitely requires refining net_busy_loop_on() to optionally
> > take some state derived from the sockets in the (e)pollset.
>
> Yes, or at least give some control to each application.
> I was thinking about this:
>
> diff --git a/fs/eventpoll.c b/fs/eventpoll.c
> index a5d219d920e7..1b104c8bda5e 100644
> --- a/fs/eventpoll.c
> +++ b/fs/eventpoll.c
> @@ -432,7 +432,7 @@ static inline void ep_set_busy_poll_napi_id(struct
> epitem *epi)
> return;
>
> sk = sock->sk;
> - if (!sk)
> + if (!sk || !sk->sk_ll_usec)
> return;
>
> napi_id = READ_ONCE(sk->sk_napi_id);
>
> With this change, busy_poll would rely on either busy_read or
> SO_BUSY_POLL.
>
> Does this make any sense?
The main issue I see would be in doing it unconditionally, changing
existing behavior.
Perhaps we can add a socket flag and replace the e()pollset busy poll
budget with sk->sk_ll_usec if set? The pollsets would need private
fields initialized from sysctl_net_busy_poll. This also allows
selectively enabling busy polling while leaving global
net_core_busy_poll zero otherwise.
This flag could for instance be passed through SO_BUSY_POLL by
accepting an optlen of 2*sizeof(int).
Just a thought. This is quickly getting a lot more complex than a nice
one-line patch.
^ permalink raw reply
* Re: [PATCH net-next v2 2/3] net: dsa: mv88e6xxx: add support for bridge flags
From: Russell King - ARM Linux admin @ 2019-02-19 17:14 UTC (permalink / raw)
To: Vivien Didelot
Cc: Andrew Lunn, Florian Fainelli, Heiner Kallweit, David S. Miller,
netdev
In-Reply-To: <20190219120000.GB4140@t480s.localdomain>
Hi Vivien,
On Tue, Feb 19, 2019 at 12:00:00PM -0500, Vivien Didelot wrote:
> Hi Russell,
>
> On Tue, 19 Feb 2019 16:24:35 +0000, Russell King - ARM Linux admin <linux@armlinux.org.uk> wrote:
> > > > +static unsigned long mv88e6xxx_bridge_flags_support(struct dsa_switch *ds)
> > > > +{
> > > > + struct mv88e6xxx_chip *chip = ds->priv;
> > > > + unsigned long support = 0;
> > > > +
> > > > + if (chip->info->ops->port_set_egress_floods)
> > > > + support |= BR_FLOOD | BR_MCAST_FLOOD;
> > > > +
> > > > + return support;
> > > > +}
> > >
> > > I think that it isn't necessary to propagate the notion of bridge flags down
> > > to the DSA drivers. It might be just enough to add:
> > >
> > > port_egress_flood(dsa_switch *ds, int port, bool uc, bool mc)
> > >
> > > to dsa_switch_ops and set BR_FLOOD | BR_MCAST_FLOOD from the DSA core,
> > > if the targeted driver has ds->ops->port_set_egress_flood. What do you think?
> >
> > There are two other flags that I haven't covered which the bridge code
> > expects to be offloaded, and those are the broadcast flood flag and
> > the learning flag.
>
> I see. What does the bridge code do if these flags are set? Does it expect
> the underlying devices to handle ff:ff:ff:ff:ff:ff magically or does it
> program this entry into the bridged ports?
The bridge code defaults to all four flags set. See new_nbp() in
net/bridge/br_if.c:
p->flags = BR_LEARNING | BR_FLOOD | BR_MCAST_FLOOD | BR_BCAST_FLOOD;
bridge(8) doesn't touch BR_BCAST_FLOOD, but it is made available to
userspace via netlink and IFLA_BRPORT_BCAST_FLOOD. Hence, there's
no man page documentation for that flag.
According to br_flood() in net/bridge/br_forward.c, it controls
whether broadcast frames are flooded to all ports or not. Changing
this flag is merely handled just like the multicast/unicast flooding
flags - a call is made to set the offloaded flags, and if it isn't
returned as being supported, a warning is printed. No attempt is
made to create or change a forwarding entry for the broadcast MAC
address.
bridge(8) does document BR_LEARNING via IFLA_BRPORT_LEARNING:
learning on or learning off
Controls whether a given port will learn MAC addresses from
received traffic or not. If learning if off, the bridge will end
up flooding any traffic for which it has no FDB entry. By
default this flag is on.
> In the latter case we have almost nothing to do. In the former case, we can
> make the core call dsa_port_mdb_add on setup and when a VLAN is added.
>
> mv88e6xxx tries to be smart and is already doing that and I'm really not a fan.
>
> If tomorrow there's a switch capable of simply toggling a bit to do that,
> we can add a new ops and skip the port_mdb_add call in the core.
>
> > I know that the Marvell switches don't have a bit to control the
> > broadcast flooding, that appears to be controlled via a static entry
> > in the ATU which would have to be modified as the broadcast flood flag
> > is manipulated. I don't know how that is handled in other bridges.
> >
> > Do we want to include the broadcast flood in the above prototype?
> > If we go for this, how do we detect which options a switch supports?
>
> If the necessary dsa_switch_ops routine is correctly prototyped, having it
> implemented by a driver or not should be enough to inform the core that the
> related feature(s) is/are supported by the switch.
>
> I'll try to give a bit more context on why I'd prefer this approach, hoping
> it makes sense: a switch driver does not need to understand bridge flags
> per-se, the core should give enough abstraction to this layer (and any other
> net-specifics). The core just needs to know if a driver can program this or
> that. More importantly, it can easily become messy to maintain switch-cases
> of arbitrary flags in all drivers and the core.
So, should we go the other way and have:
int (*port_learning)(struct dsa_switch *ds, int port, bool enable);
int (*port_egress_flood_uc)(struct dsa_switch *ds, int port, bool enable);
int (*port_egress_flood_mc)(struct dsa_switch *ds, int port, bool enable);
int (*port_egress_flood_bc)(struct dsa_switch *ds, int port, bool enable);
rather than trying to combine uc/mc into one? It would mean that we'd
be performing more bus reads/writes, but I guess that doesn't matter
for these configuration parameters.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up
^ permalink raw reply
* Re: [PATCH net-next v2 2/3] net: dsa: mv88e6xxx: add support for bridge flags
From: Vivien Didelot @ 2019-02-19 17:23 UTC (permalink / raw)
To: Russell King - ARM Linux admin
Cc: Andrew Lunn, Florian Fainelli, Heiner Kallweit, David S. Miller,
netdev
In-Reply-To: <20190219170059.h5j552kuplvdaqol@shell.armlinux.org.uk>
Hi Russell,
On Tue, 19 Feb 2019 17:00:59 +0000, Russell King - ARM Linux admin <linux@armlinux.org.uk> wrote:
> > to dsa_switch_ops and set BR_FLOOD | BR_MCAST_FLOOD from the DSA core,
> > if the targeted driver has ds->ops->port_set_egress_flood. What do you think?
>
> I've just changed my last patch to set these modes from
> dsa_port_bridge_join() and dsa_port_bridge_leave(), and while testing,
> I notice this on the ZII rev B board:
>
> At boot (without anything connected to any of the switch ports):
>
> br0: port 1(lan0) entered blocking state
> br0: port 1(lan0) entered disabled state
> device lan0 entered promiscuous mode
> device eth1 entered promiscuous mode
> br0: port 2(lan1) entered blocking state
> br0: port 2(lan1) entered disabled state
> device lan1 entered promiscuous mode
> ...
>
> I then removed lan0 from the bridge:
>
> device lan0 left promiscuous mode
> br0: port 1(lan0) entered disabled state
>
> and then added it back:
>
> br0: port 1(lan0) entered blocking state
> br0: port 1(lan0) entered disabled state
> device lan0 entered promiscuous mode
>
> Now, you'd expect lan0 and lan1 to be configured the same at this
> point, and the same as it was before lan0 was removed from the bridge?
> lan0 is port 0, lan1 is port 1 on this switch - and the register debug
> says:
>
> GLOBAL GLOBAL2 SERDES 0 1 2 3 4 5 6
> 0: c800 0 1140 500f 500f 500f 500f 500f 4e07 4d04
> ...
> 4: 40a8 258 1e0 43c 43d 43d 7c 430 53f 373f
>
> Note that port 0 is in disabled state, but port 1 and 2 are in
> blocking state... but wait, the kernel printed a message saying it was
> in disabled state!
>
> If I do the same for lan1, port 1 above changed from 0x43d to 0x433 as
> expected, and then returns to 0x43c.
>
> It looks like DSA isn't always in sync with bridge as per port state.
One thing we have to handle in DSA core are the unbridged ports. Such isolated
ports must be in forwarding state if they are up, so that they can be used
without a bridge, as a standard network interface.
Maybe it'd be simpler if the bridge code would put the interface down when
unbridging it. That way we could remove the dsa_port_set_state_now(dp,
BR_STATE_FORWARDING) hack from dsa_port_bridge_leave. Not sure if that makes
sense though.
Thanks,
Vivien
^ permalink raw reply
* Re: [PATCH RESEND net] net: phy: xgmiitorgmii: Support generic PHY status read
From: Andrew Lunn @ 2019-02-19 17:25 UTC (permalink / raw)
To: Paul Kocialkowski
Cc: Florian Fainelli, netdev, linux-arm-kernel, linux-kernel,
Michal Simek, David S . Miller, Thomas Petazzoni, Heiner Kallweit
In-Reply-To: <b0f700f85cd35bf26a97280f3bef7c4e87102d63.camel@bootlin.com>
> Thanks for the suggestion! So I had a closer look at that driver to try
> and see what could go wrong and it looks like I found a few things
> there.
Hi Paul
Yes, this driver has issues. If i remember correctly, it got merged
while i was on vacation. I pointed out a few issues, but the authors
never responded. Feel free to fix it up.
Andrew
^ permalink raw reply
* Re: [Bridge] [RFC v2] net: bridge: don't flood known multicast traffic when snooping is enabled
From: Nikolay Aleksandrov @ 2019-02-19 17:26 UTC (permalink / raw)
To: Linus Lüssing; +Cc: bridge, netdev, roopa, f.fainelli, idosch
In-Reply-To: <20190219154219.GG10191@otheros>
On 19/02/2019 17:42, Linus Lüssing wrote:
> On Tue, Feb 19, 2019 at 03:31:42PM +0200, Nikolay Aleksandrov wrote:
>> On 19/02/2019 11:21, Linus Lüssing wrote:
>>> On Tue, Feb 19, 2019 at 09:57:16AM +0100, Linus Lüssing wrote:
>>>> On Mon, Feb 18, 2019 at 02:21:07PM +0200, Nikolay Aleksandrov wrote:
>>>>> This is v2 of the RFC patch which aims to forward packets to known
>>>>> mdsts' ports only (the no querier case). After v1 I've kept
>>>>> the previous behaviour when it comes to unregistered traffic or when
>>>>> a querier is present. All of this is of course only with snooping
>>>>> enabled. So with this patch the following changes should occur:
>>>>> - No querier: forward known mdst traffic to its registered ports,
>>>>> no change about unknown mcast (flood)
>>>>> - Querier present: no change
>>>>>
>>>>> The reason to do this is simple - we want to respect the user's mdb
>>>>> configuration in both cases, that is if the user adds static mdb entries
>>>>> manually then we should use that information about forwarding traffic.
>>>>>
>>>>> What do you think ?
>>>>>
>>>>> * Notes
>>>>> Traffic that is currently marked as mrouters_only:
>>>>> - IPv4: non-local mcast traffic, igmp reports
>>>>> - IPv6: non-all-nodes-dst mcast traffic, mldv1 reports
>>>>>
>>>>> Simple use case:
>>>>> $ echo 1 > /sys/class/net/bridge/bridge/multicast_snooping
>>>>> $ bridge mdb add dev bridge port swp1 grp 239.0.0.1
>>>>> - without a querier currently traffic for 239.0.0.1 will still be flooded,
>>>>> with this change it will be forwarded only to swp1
>>>>
>>>> There is still the issue with unsolicited reports adding mdst
>>>> entries here, too. Leading to unwanted packet loss and connectivity issues.
>>>
>>> Or in other words, an unsolicited report will turn a previously
>>> unregistered multicast group into a registered one. However in the
>>> absence of a querier the knowledge about this newly registered multicast group
>>> will be incomplete. And therefore still needs to be flooded to avoid packet
>>> loss.
>>>
>>
>> Right, this is expected. If the user has enabled igmp snooping and doesn't have
>> a querier present then such behaviour is to be expected.
>
> IGMP snooping is currently enabled by default. And IGMP/MLD
> querier is disabled by default. I wouldn't want packet loss to be
> the expected behaviour in a default setup.
>
> This default setup currently works. However with this change it
> will introduce packet loss, as you acknowledged (if I understood
> you correctly?).
>
Yeah, I'm not pushing this change anymore, the only option was to add
a tunable for the behaviour which I described in my previous reply but I
really want to avoid adding yet another bridge option as it has
way too many already.
>> What is surprising is
>> the user explicitly enabling igmp snooping, adding an mdst and then still
>> getting it flooded. :)
>
> Hm, for me that does not seem surprising. I would not expect igmp
> snooping to work without a querier on the link. Why don't you just
> add/enable a querier in your setups then if you want to avoid
> flooding?
>
Yep, ack.
>
>> An alternative is to drop all unregistered traffic when a querier is not present.
>> But that will surely break setups and at best should be a configurable option that
>> is disabled by default.
>
> Absolutely right. Always dropping with no querier is no option. That's why I'd say
> you should always flood multicast packets if there is no querier.
>
>
>> So in effect and to try and make everybody happy we can add an option to control
>> this behaviour with keeping the current as default and adding the following options:
>> - no querier: flood all (default, current)
>
> ACK
>
> For the other options maybe I do not understand your scenario yet.
> Wouldn't these two options result in eratic behaviour?
>
>> - no querier: flood unregistered, forward registered
>> - no querier: drop unregistered, forward registered
>
To be fair I've seen all 3 options being default on different versions of
network OSes by major vendors.
> Let's call these two cases A) - flood unregistered, forward
> registered and B) - drop unregistered, forward registered.
>
>
> Let's say you have a bridge with two ports:
> (1)<-[br]->(2). And no querier.
>
> Behind (2) there is a listener for group M. M is not
> registered by the bridge because either that listener joined
> before the bridge came up or the entry was registered once but had
> timed out. Or there was packet loss and the report did not arrive
> at the bridge (for instance bc. listener is behind a wireless
> connection).
>
> For case B) we can immediately see that the listener at (2) will
> not receive the traffic it signed up for. And this is a permanent
> issue as the listener will not send any further reports.
>
> Case A) is ok, the listener behind port (2) receives its traffic.
>
>
> Now, a listener for M joins at (1). It sends an unsolicited
> report. Group M becomes registered by the bridge. Both for
> cases (A) and (B) this new listener at (1) will receive its
> traffic. However, not only in case B) now, but in case A), too,
> the listener at (2) will rceive no more traffic for M.
>
>
> Now 260 seconds pass (multicast_membership_interval). The entry
> for M times out and is deleted on the bridge. It becomes
> unregistered.
>
> Now for case (A) things would be ok again, both listeners at (1)
> and (2) would receive traffic. For now - as long as no new listener
> joins again.
>
> For case (B), both the listener at port (1) and (2) will fail to
> receive the traffic they signed up for.
>
>
> ---
>
> I hope this illustrates a bit what I'm afraid of? If you have any
> measures to prevent such behavior in your setup, I'd be curious to
> know.
>
It does and I'm aware of these scenarios, but there're static mcast trees
and also such that are controlled by user-space daemons, that is the daemon
keeps refreshing the mdb entries. They're non-standard for sure and obviously
we suggest having a querier always. That's why I suggested having these
as options, but now I'm retracting that and will table this whole thing for
the time being.
I really don't want to add another option to the bridge lightly.
> Regards, Linus
>
Thanks for the discussion and all the helpful comments,
Nik
^ permalink raw reply
* Re: [PATCH net-next v2 2/3] net: dsa: mv88e6xxx: add support for bridge flags
From: Russell King - ARM Linux admin @ 2019-02-19 17:27 UTC (permalink / raw)
To: Vivien Didelot
Cc: Andrew Lunn, Florian Fainelli, Heiner Kallweit, David S. Miller,
netdev
In-Reply-To: <20190219122304.GB10959@t480s.localdomain>
On Tue, Feb 19, 2019 at 12:23:04PM -0500, Vivien Didelot wrote:
> Hi Russell,
>
> On Tue, 19 Feb 2019 17:00:59 +0000, Russell King - ARM Linux admin <linux@armlinux.org.uk> wrote:
> > > to dsa_switch_ops and set BR_FLOOD | BR_MCAST_FLOOD from the DSA core,
> > > if the targeted driver has ds->ops->port_set_egress_flood. What do you think?
> >
> > I've just changed my last patch to set these modes from
> > dsa_port_bridge_join() and dsa_port_bridge_leave(), and while testing,
> > I notice this on the ZII rev B board:
> >
> > At boot (without anything connected to any of the switch ports):
> >
> > br0: port 1(lan0) entered blocking state
> > br0: port 1(lan0) entered disabled state
> > device lan0 entered promiscuous mode
> > device eth1 entered promiscuous mode
> > br0: port 2(lan1) entered blocking state
> > br0: port 2(lan1) entered disabled state
> > device lan1 entered promiscuous mode
> > ...
> >
> > I then removed lan0 from the bridge:
> >
> > device lan0 left promiscuous mode
> > br0: port 1(lan0) entered disabled state
> >
> > and then added it back:
> >
> > br0: port 1(lan0) entered blocking state
> > br0: port 1(lan0) entered disabled state
> > device lan0 entered promiscuous mode
> >
> > Now, you'd expect lan0 and lan1 to be configured the same at this
> > point, and the same as it was before lan0 was removed from the bridge?
> > lan0 is port 0, lan1 is port 1 on this switch - and the register debug
> > says:
> >
> > GLOBAL GLOBAL2 SERDES 0 1 2 3 4 5 6
> > 0: c800 0 1140 500f 500f 500f 500f 500f 4e07 4d04
> > ...
> > 4: 40a8 258 1e0 43c 43d 43d 7c 430 53f 373f
> >
> > Note that port 0 is in disabled state, but port 1 and 2 are in
> > blocking state... but wait, the kernel printed a message saying it was
> > in disabled state!
> >
> > If I do the same for lan1, port 1 above changed from 0x43d to 0x433 as
> > expected, and then returns to 0x43c.
> >
> > It looks like DSA isn't always in sync with bridge as per port state.
>
> One thing we have to handle in DSA core are the unbridged ports. Such isolated
> ports must be in forwarding state if they are up, so that they can be used
> without a bridge, as a standard network interface.
>
> Maybe it'd be simpler if the bridge code would put the interface down when
> unbridging it. That way we could remove the dsa_port_set_state_now(dp,
> BR_STATE_FORWARDING) hack from dsa_port_bridge_leave. Not sure if that makes
> sense though.
I'm not concerned about what happens when the port is removed from the
bridge above.
What I'm concerned about is the two different states when the port is
part of a bridge.
First time, the DSA switch is set to blocking mode, despite the kernel
saying the port is disabled.
Second time, the DSA switch is set to disabled mode, and the kernel
says the port is disabled.
Why is the port in blocking mode first time around?
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up
^ permalink raw reply
* Re: [PATCH][net-next] bridge: remove redundant check on err in br_multicast_ipv4_rcv
From: Roopa Prabhu @ 2019-02-19 17:27 UTC (permalink / raw)
To: Li RongQing; +Cc: netdev, Nikolay Aleksandrov
In-Reply-To: <1550542629-17121-1-git-send-email-lirongqing@baidu.com>
On Mon, Feb 18, 2019 at 6:17 PM Li RongQing <lirongqing@baidu.com> wrote:
>
> br_ip4_multicast_mrd_rcv only return 0 and -ENOMSG,
> no other negative value
>
> Signed-off-by: Li RongQing <lirongqing@baidu.com>
Acked-by: Roopa Prabhu <roopa@cumulusnetworks.com>
looks fine to me. CC Nikolay
> net/bridge/br_multicast.c | 7 +------
> 1 file changed, 1 insertion(+), 6 deletions(-)
>
> diff --git a/net/bridge/br_multicast.c b/net/bridge/br_multicast.c
> index 4a048fd1cbea..fe9f2d8ca2c1 100644
> --- a/net/bridge/br_multicast.c
> +++ b/net/bridge/br_multicast.c
> @@ -1615,12 +1615,7 @@ static int br_multicast_ipv4_rcv(struct net_bridge *br,
> if (ip_hdr(skb)->protocol == IPPROTO_PIM)
> br_multicast_pim(br, port, skb);
> } else if (ipv4_is_all_snoopers(ip_hdr(skb)->daddr)) {
> - err = br_ip4_multicast_mrd_rcv(br, port, skb);
> -
> - if (err < 0 && err != -ENOMSG) {
> - br_multicast_err_count(br, port, skb->protocol);
> - return err;
> - }
> + br_ip4_multicast_mrd_rcv(br, port, skb);
> }
>
> return 0;
> --
> 2.16.2
>
^ permalink raw reply
* Re: [PATCH net-next v2 2/3] net: dsa: mv88e6xxx: add support for bridge flags
From: Vivien Didelot @ 2019-02-19 17:38 UTC (permalink / raw)
To: Russell King - ARM Linux admin
Cc: Andrew Lunn, Florian Fainelli, Heiner Kallweit, David S. Miller,
netdev
In-Reply-To: <20190219171414.cvaiw7u2xnd5zk3g@shell.armlinux.org.uk>
Hi Russell,
On Tue, 19 Feb 2019 17:14:14 +0000, Russell King - ARM Linux admin <linux@armlinux.org.uk> wrote:
> > > > > +static unsigned long mv88e6xxx_bridge_flags_support(struct dsa_switch *ds)
> > > > > +{
> > > > > + struct mv88e6xxx_chip *chip = ds->priv;
> > > > > + unsigned long support = 0;
> > > > > +
> > > > > + if (chip->info->ops->port_set_egress_floods)
> > > > > + support |= BR_FLOOD | BR_MCAST_FLOOD;
> > > > > +
> > > > > + return support;
> > > > > +}
> > > >
> > > > I think that it isn't necessary to propagate the notion of bridge flags down
> > > > to the DSA drivers. It might be just enough to add:
> > > >
> > > > port_egress_flood(dsa_switch *ds, int port, bool uc, bool mc)
> > > >
> > > > to dsa_switch_ops and set BR_FLOOD | BR_MCAST_FLOOD from the DSA core,
> > > > if the targeted driver has ds->ops->port_set_egress_flood. What do you think?
> > >
> > > There are two other flags that I haven't covered which the bridge code
> > > expects to be offloaded, and those are the broadcast flood flag and
> > > the learning flag.
> >
> > I see. What does the bridge code do if these flags are set? Does it expect
> > the underlying devices to handle ff:ff:ff:ff:ff:ff magically or does it
> > program this entry into the bridged ports?
>
> The bridge code defaults to all four flags set. See new_nbp() in
> net/bridge/br_if.c:
>
> p->flags = BR_LEARNING | BR_FLOOD | BR_MCAST_FLOOD | BR_BCAST_FLOOD;
>
> bridge(8) doesn't touch BR_BCAST_FLOOD, but it is made available to
> userspace via netlink and IFLA_BRPORT_BCAST_FLOOD. Hence, there's
> no man page documentation for that flag.
>
> According to br_flood() in net/bridge/br_forward.c, it controls
> whether broadcast frames are flooded to all ports or not. Changing
> this flag is merely handled just like the multicast/unicast flooding
> flags - a call is made to set the offloaded flags, and if it isn't
> returned as being supported, a warning is printed. No attempt is
> made to create or change a forwarding entry for the broadcast MAC
> address.
OK, thanks for the details. The programming of the broadcast MAC address
must be handled in the core then, I will move this from mv88e6xxx up to the
DSA layer later, but that's totally orthogonal here.
>
> bridge(8) does document BR_LEARNING via IFLA_BRPORT_LEARNING:
>
> learning on or learning off
> Controls whether a given port will learn MAC addresses from
> received traffic or not. If learning if off, the bridge will end
> up flooding any traffic for which it has no FDB entry. By
> default this flag is on.
>
> > In the latter case we have almost nothing to do. In the former case, we can
> > make the core call dsa_port_mdb_add on setup and when a VLAN is added.
> >
> > mv88e6xxx tries to be smart and is already doing that and I'm really not a fan.
> >
> > If tomorrow there's a switch capable of simply toggling a bit to do that,
> > we can add a new ops and skip the port_mdb_add call in the core.
> >
> > > I know that the Marvell switches don't have a bit to control the
> > > broadcast flooding, that appears to be controlled via a static entry
> > > in the ATU which would have to be modified as the broadcast flood flag
> > > is manipulated. I don't know how that is handled in other bridges.
> > >
> > > Do we want to include the broadcast flood in the above prototype?
> > > If we go for this, how do we detect which options a switch supports?
> >
> > If the necessary dsa_switch_ops routine is correctly prototyped, having it
> > implemented by a driver or not should be enough to inform the core that the
> > related feature(s) is/are supported by the switch.
> >
> > I'll try to give a bit more context on why I'd prefer this approach, hoping
> > it makes sense: a switch driver does not need to understand bridge flags
> > per-se, the core should give enough abstraction to this layer (and any other
> > net-specifics). The core just needs to know if a driver can program this or
> > that. More importantly, it can easily become messy to maintain switch-cases
> > of arbitrary flags in all drivers and the core.
>
> So, should we go the other way and have:
>
> int (*port_learning)(struct dsa_switch *ds, int port, bool enable);
> int (*port_egress_flood_uc)(struct dsa_switch *ds, int port, bool enable);
> int (*port_egress_flood_mc)(struct dsa_switch *ds, int port, bool enable);
> int (*port_egress_flood_bc)(struct dsa_switch *ds, int port, bool enable);
>
> rather than trying to combine uc/mc into one? It would mean that we'd
> be performing more bus reads/writes, but I guess that doesn't matter
> for these configuration parameters.
I like this very much. As long as these flags can be programmed in switch
devices, these ops totally make sense.
Thanks,
Vivien
^ permalink raw reply
* Re: [PATCH net-next v2 2/3] net: dsa: mv88e6xxx: add support for bridge flags
From: Florian Fainelli @ 2019-02-19 17:44 UTC (permalink / raw)
To: Vivien Didelot, Russell King - ARM Linux admin
Cc: Andrew Lunn, Heiner Kallweit, David S. Miller, netdev
In-Reply-To: <20190219123828.GD10959@t480s.localdomain>
On 2/19/19 9:38 AM, Vivien Didelot wrote:
> Hi Russell,
>
> On Tue, 19 Feb 2019 17:14:14 +0000, Russell King - ARM Linux admin <linux@armlinux.org.uk> wrote:
>>>>>> +static unsigned long mv88e6xxx_bridge_flags_support(struct dsa_switch *ds)
>>>>>> +{
>>>>>> + struct mv88e6xxx_chip *chip = ds->priv;
>>>>>> + unsigned long support = 0;
>>>>>> +
>>>>>> + if (chip->info->ops->port_set_egress_floods)
>>>>>> + support |= BR_FLOOD | BR_MCAST_FLOOD;
>>>>>> +
>>>>>> + return support;
>>>>>> +}
>>>>>
>>>>> I think that it isn't necessary to propagate the notion of bridge flags down
>>>>> to the DSA drivers. It might be just enough to add:
>>>>>
>>>>> port_egress_flood(dsa_switch *ds, int port, bool uc, bool mc)
>>>>>
>>>>> to dsa_switch_ops and set BR_FLOOD | BR_MCAST_FLOOD from the DSA core,
>>>>> if the targeted driver has ds->ops->port_set_egress_flood. What do you think?
>>>>
>>>> There are two other flags that I haven't covered which the bridge code
>>>> expects to be offloaded, and those are the broadcast flood flag and
>>>> the learning flag.
>>>
>>> I see. What does the bridge code do if these flags are set? Does it expect
>>> the underlying devices to handle ff:ff:ff:ff:ff:ff magically or does it
>>> program this entry into the bridged ports?
>>
>> The bridge code defaults to all four flags set. See new_nbp() in
>> net/bridge/br_if.c:
>>
>> p->flags = BR_LEARNING | BR_FLOOD | BR_MCAST_FLOOD | BR_BCAST_FLOOD;
>>
>> bridge(8) doesn't touch BR_BCAST_FLOOD, but it is made available to
>> userspace via netlink and IFLA_BRPORT_BCAST_FLOOD. Hence, there's
>> no man page documentation for that flag.
>>
>> According to br_flood() in net/bridge/br_forward.c, it controls
>> whether broadcast frames are flooded to all ports or not. Changing
>> this flag is merely handled just like the multicast/unicast flooding
>> flags - a call is made to set the offloaded flags, and if it isn't
>> returned as being supported, a warning is printed. No attempt is
>> made to create or change a forwarding entry for the broadcast MAC
>> address.
>
> OK, thanks for the details. The programming of the broadcast MAC address
> must be handled in the core then, I will move this from mv88e6xxx up to the
> DSA layer later, but that's totally orthogonal here.
I am not sure if it makes sense for us to work hard on supporting
BR_BCAST_FLOOD, for instance, on Broadcom switches, there does not
appear to be an easy way to specify whether broadcast traffic will be
flooded or not, it will be. The only way to tsolve that is to create a
MDB/FDB entry with negative logic (e.g.: forward to a
non-existing/connected port). Every other bridge flag typically maps 1:1
with a corresponding hardware feature, so we should support them.
>
>>
>> bridge(8) does document BR_LEARNING via IFLA_BRPORT_LEARNING:
>>
>> learning on or learning off
>> Controls whether a given port will learn MAC addresses from
>> received traffic or not. If learning if off, the bridge will end
>> up flooding any traffic for which it has no FDB entry. By
>> default this flag is on.
>>
>>> In the latter case we have almost nothing to do. In the former case, we can
>>> make the core call dsa_port_mdb_add on setup and when a VLAN is added.
>>>
>>> mv88e6xxx tries to be smart and is already doing that and I'm really not a fan.
>>>
>>> If tomorrow there's a switch capable of simply toggling a bit to do that,
>>> we can add a new ops and skip the port_mdb_add call in the core.
>>>
>>>> I know that the Marvell switches don't have a bit to control the
>>>> broadcast flooding, that appears to be controlled via a static entry
>>>> in the ATU which would have to be modified as the broadcast flood flag
>>>> is manipulated. I don't know how that is handled in other bridges.
>>>>
>>>> Do we want to include the broadcast flood in the above prototype?
>>>> If we go for this, how do we detect which options a switch supports?
>>>
>>> If the necessary dsa_switch_ops routine is correctly prototyped, having it
>>> implemented by a driver or not should be enough to inform the core that the
>>> related feature(s) is/are supported by the switch.
>>>
>>> I'll try to give a bit more context on why I'd prefer this approach, hoping
>>> it makes sense: a switch driver does not need to understand bridge flags
>>> per-se, the core should give enough abstraction to this layer (and any other
>>> net-specifics). The core just needs to know if a driver can program this or
>>> that. More importantly, it can easily become messy to maintain switch-cases
>>> of arbitrary flags in all drivers and the core.
>>
>> So, should we go the other way and have:
>>
>> int (*port_learning)(struct dsa_switch *ds, int port, bool enable);
>> int (*port_egress_flood_uc)(struct dsa_switch *ds, int port, bool enable);
>> int (*port_egress_flood_mc)(struct dsa_switch *ds, int port, bool enable);
>> int (*port_egress_flood_bc)(struct dsa_switch *ds, int port, bool enable);
>>
>> rather than trying to combine uc/mc into one? It would mean that we'd
>> be performing more bus reads/writes, but I guess that doesn't matter
>> for these configuration parameters.
>
> I like this very much. As long as these flags can be programmed in switch
> devices, these ops totally make sense.
No objections or preference here, whatever works :)
--
Florian
^ permalink raw reply
* Re: [PATCH v3] net: ns83820: code cleanup for ns83820_probe_phy()
From: Andrew Lunn @ 2019-02-19 17:46 UTC (permalink / raw)
To: Walter Harms
Cc: Mao Wenan, kernel-janitors, netdev, john.fastabend, hawk,
jakub.kicinski, daniel, ast, julia.lawall
In-Reply-To: <715767253.151509.1550580124264@ox-groupware.bfs.de>
> > for (i=1; i<2; i++) {
>
>
> the loop here seems also pointless, so you can eliminate i.
> (or did i muss something ?)
If you widen out your view a bit, you find all this code is inside a
#ifdef PHY_CODE_IS_FINISHED. I don't see anything which actually
defines that.
So a lot more code could probably be removed.
Andrew
^ permalink raw reply
* Re: L2TPv3 offset
From: Guillaume Nault @ 2019-02-19 17:54 UTC (permalink / raw)
To: James Chapman; +Cc: t.martitz, davem, netdev
In-Reply-To: <e685d22c-e28e-8318-f612-07e2d1cd03ab@katalix.com>
On Tue, Feb 19, 2019 at 04:36:55PM +0000, James Chapman wrote:
> On 19/02/2019 13:09, t.martitz@avm.de wrote:
> >
> > Because everytime a LCCE decapsulates such traffic it'll suffer from
> > unaligned access to the inner IP header (likewise for the outer IP
> > header when encapsulating). It's a fundamental assumption that the IP
> > header is word-aligned in Linux so I'm surprised this isn't solved
> > already. And now the only way "fix" without patching the kernel is
> > being removed.
> >
IIUC, you'd be perfectly fine running without offset if the stack was
properly handling the alignment problems created by the encapsulated
ethernet header.
Unaligned headers is a fundamental problem when tunneling ethernet
frames. I'd expect other encapsulation modules (vxlan, geneve,
gretap...) to have the same issues (but I'd be happy to be proven
wrong).
The topic has been discussed a few years ago:
https://lore.kernel.org/netdev/20160922.015242.735026657310158125.davem@davemloft.net/
I might very well have missed some discussions, but I don't remember
any patch aimed at tackling this problem so far.
^ permalink raw reply
* [PATCH net] team: use operstate consistently for linkup
From: George Wilkie @ 2019-02-19 15:57 UTC (permalink / raw)
To: Jiri Pirko, David S. Miller; +Cc: netdev
When a port is added to a team, its initial state is derived
from netif_carrier_ok rather than netif_oper_up.
If it is carrier up but operationally down at the time of being
added, the port state.linkup will be set prematurely.
port state.linkup should be set consistently using
netif_oper_up rather than netif_carrier_ok.
Fixes: f1d22a1e0595 ("team: account for oper state")
Signed-off-by: George Wilkie <gwilkie@vyatta.att-mail.com>
---
drivers/net/team/team.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/net/team/team.c b/drivers/net/team/team.c
index 958f1cf67282..6ce3f666d142 100644
--- a/drivers/net/team/team.c
+++ b/drivers/net/team/team.c
@@ -1256,7 +1256,7 @@ static int team_port_add(struct team *team, struct net_device *port_dev,
list_add_tail_rcu(&port->list, &team->port_list);
team_port_enable(team, port);
__team_compute_features(team);
- __team_port_change_port_added(port, !!netif_carrier_ok(port_dev));
+ __team_port_change_port_added(port, !!netif_oper_up(port_dev));
__team_options_change_check(team);
netdev_info(dev, "Port device %s added\n", portname);
@@ -2915,7 +2915,7 @@ static int team_device_event(struct notifier_block *unused,
switch (event) {
case NETDEV_UP:
- if (netif_carrier_ok(dev))
+ if (netif_oper_up(dev))
team_port_change_check(port, true);
break;
case NETDEV_DOWN:
--
2.11.0
^ permalink raw reply related
* Re: [PATCH net-next v2 2/3] net: dsa: mv88e6xxx: add support for bridge flags
From: Russell King - ARM Linux admin @ 2019-02-19 18:08 UTC (permalink / raw)
To: Vivien Didelot
Cc: Andrew Lunn, Florian Fainelli, Heiner Kallweit, David S. Miller,
netdev
In-Reply-To: <20190219123828.GD10959@t480s.localdomain>
On Tue, Feb 19, 2019 at 12:38:28PM -0500, Vivien Didelot wrote:
> Hi Russell,
>
> On Tue, 19 Feb 2019 17:14:14 +0000, Russell King - ARM Linux admin <linux@armlinux.org.uk> wrote:
> > > > > > +static unsigned long mv88e6xxx_bridge_flags_support(struct dsa_switch *ds)
> > > > > > +{
> > > > > > + struct mv88e6xxx_chip *chip = ds->priv;
> > > > > > + unsigned long support = 0;
> > > > > > +
> > > > > > + if (chip->info->ops->port_set_egress_floods)
> > > > > > + support |= BR_FLOOD | BR_MCAST_FLOOD;
> > > > > > +
> > > > > > + return support;
> > > > > > +}
> > > > >
> > > > > I think that it isn't necessary to propagate the notion of bridge flags down
> > > > > to the DSA drivers. It might be just enough to add:
> > > > >
> > > > > port_egress_flood(dsa_switch *ds, int port, bool uc, bool mc)
> > > > >
> > > > > to dsa_switch_ops and set BR_FLOOD | BR_MCAST_FLOOD from the DSA core,
> > > > > if the targeted driver has ds->ops->port_set_egress_flood. What do you think?
> > > >
> > > > There are two other flags that I haven't covered which the bridge code
> > > > expects to be offloaded, and those are the broadcast flood flag and
> > > > the learning flag.
> > >
> > > I see. What does the bridge code do if these flags are set? Does it expect
> > > the underlying devices to handle ff:ff:ff:ff:ff:ff magically or does it
> > > program this entry into the bridged ports?
> >
> > The bridge code defaults to all four flags set. See new_nbp() in
> > net/bridge/br_if.c:
> >
> > p->flags = BR_LEARNING | BR_FLOOD | BR_MCAST_FLOOD | BR_BCAST_FLOOD;
> >
> > bridge(8) doesn't touch BR_BCAST_FLOOD, but it is made available to
> > userspace via netlink and IFLA_BRPORT_BCAST_FLOOD. Hence, there's
> > no man page documentation for that flag.
> >
> > According to br_flood() in net/bridge/br_forward.c, it controls
> > whether broadcast frames are flooded to all ports or not. Changing
> > this flag is merely handled just like the multicast/unicast flooding
> > flags - a call is made to set the offloaded flags, and if it isn't
> > returned as being supported, a warning is printed. No attempt is
> > made to create or change a forwarding entry for the broadcast MAC
> > address.
>
> OK, thanks for the details. The programming of the broadcast MAC address
> must be handled in the core then, I will move this from mv88e6xxx up to the
> DSA layer later, but that's totally orthogonal here.
>
> >
> > bridge(8) does document BR_LEARNING via IFLA_BRPORT_LEARNING:
> >
> > learning on or learning off
> > Controls whether a given port will learn MAC addresses from
> > received traffic or not. If learning if off, the bridge will end
> > up flooding any traffic for which it has no FDB entry. By
> > default this flag is on.
> >
> > > In the latter case we have almost nothing to do. In the former case, we can
> > > make the core call dsa_port_mdb_add on setup and when a VLAN is added.
> > >
> > > mv88e6xxx tries to be smart and is already doing that and I'm really not a fan.
> > >
> > > If tomorrow there's a switch capable of simply toggling a bit to do that,
> > > we can add a new ops and skip the port_mdb_add call in the core.
> > >
> > > > I know that the Marvell switches don't have a bit to control the
> > > > broadcast flooding, that appears to be controlled via a static entry
> > > > in the ATU which would have to be modified as the broadcast flood flag
> > > > is manipulated. I don't know how that is handled in other bridges.
> > > >
> > > > Do we want to include the broadcast flood in the above prototype?
> > > > If we go for this, how do we detect which options a switch supports?
> > >
> > > If the necessary dsa_switch_ops routine is correctly prototyped, having it
> > > implemented by a driver or not should be enough to inform the core that the
> > > related feature(s) is/are supported by the switch.
> > >
> > > I'll try to give a bit more context on why I'd prefer this approach, hoping
> > > it makes sense: a switch driver does not need to understand bridge flags
> > > per-se, the core should give enough abstraction to this layer (and any other
> > > net-specifics). The core just needs to know if a driver can program this or
> > > that. More importantly, it can easily become messy to maintain switch-cases
> > > of arbitrary flags in all drivers and the core.
> >
> > So, should we go the other way and have:
> >
> > int (*port_learning)(struct dsa_switch *ds, int port, bool enable);
> > int (*port_egress_flood_uc)(struct dsa_switch *ds, int port, bool enable);
> > int (*port_egress_flood_mc)(struct dsa_switch *ds, int port, bool enable);
> > int (*port_egress_flood_bc)(struct dsa_switch *ds, int port, bool enable);
> >
> > rather than trying to combine uc/mc into one? It would mean that we'd
> > be performing more bus reads/writes, but I guess that doesn't matter
> > for these configuration parameters.
>
> I like this very much. As long as these flags can be programmed in switch
> devices, these ops totally make sense.
Having these as separate functions means that we would then need
additional complexity in mv88e6xxx to store the per-port flooding state,
so we can do this:
reg &= ~MV88E6352_PORT_CTL0_EGRESS_FLOODS_MASK;
if (unicast && multicast)
reg |= MV88E6352_PORT_CTL0_EGRESS_FLOODS_ALL_UNKNOWN_DA;
else if (unicast)
reg |= MV88E6352_PORT_CTL0_EGRESS_FLOODS_NO_UNKNOWN_MC_DA;
else if (multicast)
reg |= MV88E6352_PORT_CTL0_EGRESS_FLOODS_NO_UNKNOWN_UC_DA;
else
reg |= MV88E6352_PORT_CTL0_EGRESS_FLOODS_NO_UNKNOWN_DA;
for some of the switches. It looks to me like mv88e6xxx would prefer
having at least both the unicast and multicast flags together.
Even without that, it means more code in mv88e6xxx to wrap each of
these calls between the DSA ops and the chip specific ops...
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up
^ permalink raw reply
* Re: [PATCH net-next 1/2] net/mlx5e: Introduce mlx5e_flow_esw_attr_init() helper
From: Saeed Mahameed @ 2019-02-19 18:09 UTC (permalink / raw)
To: xiangxia.m.yue@gmail.com, gerlitz.or@gmail.com; +Cc: netdev@vger.kernel.org
In-Reply-To: <1549942783-56833-1-git-send-email-xiangxia.m.yue@gmail.com>
On Mon, 2019-02-11 at 19:39 -0800, xiangxia.m.yue@gmail.com wrote:
> From: Tonghao Zhang <xiangxia.m.yue@gmail.com>
>
> Introduce the mlx5e_flow_esw_attr_init() helper
> for simplifying codes.
>
> Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com>
> ---
Applied to net-next-mlx5
Thanks!
^ permalink raw reply
* Re: [PATCH net-next 2/2] net/mlx5e: Remove 'parse_attr' argument in mlx5e_tc_add_fdb_flow()
From: Saeed Mahameed @ 2019-02-19 18:09 UTC (permalink / raw)
To: xiangxia.m.yue@gmail.com, gerlitz.or@gmail.com; +Cc: netdev@vger.kernel.org
In-Reply-To: <1549942783-56833-2-git-send-email-xiangxia.m.yue@gmail.com>
On Mon, 2019-02-11 at 19:39 -0800, xiangxia.m.yue@gmail.com wrote:
> From: Tonghao Zhang <xiangxia.m.yue@gmail.com>
>
> This patch is a little improvement. Simplify the
> mlx5e_tc_add_fdb_flow().
>
> Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com>
> ---
LGTM
Applied to net-next-mlx5
Thanks!
^ permalink raw reply
* Re: [PATCH net-next 0/2] Fix W=1 compilation warnings
From: Saeed Mahameed @ 2019-02-19 18:13 UTC (permalink / raw)
To: davem@davemloft.net, leon@kernel.org
Cc: netdev@vger.kernel.org, Leon Romanovsky,
linux-rdma@vger.kernel.org, Eyal Davidovich
In-Reply-To: <20190217132128.9709-1-leon@kernel.org>
On Sun, 2019-02-17 at 15:21 +0200, Leon Romanovsky wrote:
> From: Leon Romanovsky <leonro@mellanox.com>
>
> Hi Saeed,
>
> This short series fixes two small compilation warnings which are
> visible
> while compiling with W=1.
>
> Thanks
>
> Leon Romanovsky (2):
> net/mlx5e: Add missing static function annotation
> net/mlx5: Delete unused FPGA QPN variable
>
> drivers/net/ethernet/mellanox/mlx5/core/en/monitor_stats.c | 2 +-
> drivers/net/ethernet/mellanox/mlx5/core/fpga/core.c | 2 --
> 2 files changed, 1 insertion(+), 3 deletions(-)
>
> --
> 2.19.1
>
Series applied to net-next-mlx5
Thanks Leon!
^ permalink raw reply
* Re: [PATCH net-next v2 2/3] net: dsa: mv88e6xxx: add support for bridge flags
From: Vivien Didelot @ 2019-02-19 18:20 UTC (permalink / raw)
To: Florian Fainelli
Cc: Russell King - ARM Linux admin, Andrew Lunn, Heiner Kallweit,
David S. Miller, netdev
In-Reply-To: <a93298cc-7acd-6357-fcee-fe238487c143@gmail.com>
Hi Florian,
On Tue, 19 Feb 2019 09:44:32 -0800, Florian Fainelli <f.fainelli@gmail.com> wrote:
> > OK, thanks for the details. The programming of the broadcast MAC address
> > must be handled in the core then, I will move this from mv88e6xxx up to the
> > DSA layer later, but that's totally orthogonal here.
>
> I am not sure if it makes sense for us to work hard on supporting
> BR_BCAST_FLOOD, for instance, on Broadcom switches, there does not
> appear to be an easy way to specify whether broadcast traffic will be
> flooded or not, it will be. The only way to tsolve that is to create a
> MDB/FDB entry with negative logic (e.g.: forward to a
> non-existing/connected port). Every other bridge flag typically maps 1:1
> with a corresponding hardware feature, so we should support them.
I think it's best to keep away from driver-specific hacks :-)
Since broadcom floods multicast and BR_BCAST_FLOOD is set by default,
this would simply translate as not implementing a port_flood_bc ops in the
broadcom driver.
But I agree that it doesn't seem necessary to implement support for this yet.
Thanks,
Vivien
^ permalink raw reply
* Re: [PATCH iproute2] ip-rule/trivial: add comment about json key "to_tbl" for unspecific rule action
From: Stephen Hemminger @ 2019-02-19 18:19 UTC (permalink / raw)
To: Thomas Haller; +Cc: netdev
In-Reply-To: <20190219140331.16630-1-thaller@redhat.com>
On Tue, 19 Feb 2019 15:03:31 +0100
Thomas Haller <thaller@redhat.com> wrote:
> The key should not be called "to_tbl" because it is exactly
> not a FR_ACT_TO_TBL action.
>
> # ip rule add blackhole
> # ip -j rule | python -m json.tool
> ...
> {
> "priority": 0,
> "src": "all",
> "to_tbl": "blackhole"
> },
>
> Still, as this is already released API from v4.17.0, stick to it.
> Only add a comment for this oddity.
>
> Related: 0dd4ccc56c0e ("iprule: add json support")
>
> Signed-off-by: Thomas Haller <thaller@redhat.com>
> ---
> If it is still permissible to change the API, I can also send a patch to
> rename "to_tbl" key to "action".
>
> ip/iprule.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/ip/iprule.c b/ip/iprule.c
> index 2f58d8c2..aea175aa 100644
> --- a/ip/iprule.c
> +++ b/ip/iprule.c
> @@ -459,6 +459,8 @@ int print_rule(struct nlmsghdr *n, void *arg)
> } else if (frh->action == FR_ACT_NOP) {
> print_null(PRINT_ANY, "nop", "nop", NULL);
> } else if (frh->action != FR_ACT_TO_TBL) {
> + /* The action is not(!) to-tbl, however for historic
> + * reasons, this JSON key is called "to_tbl". */
> print_string(PRINT_ANY, "to_tbl", "%s",
> rtnl_rtntype_n2a(frh->action, b1, sizeof(b1)));
> }
Just fix it and add fixes tag. JSON has been only added for a short time.
^ permalink raw reply
* [PATCH] iwlwifi: mvm: Use div64_s64 instead of do_div in iwl_mvm_debug_range_resp
From: Nathan Chancellor @ 2019-02-19 18:21 UTC (permalink / raw)
To: Johannes Berg, Emmanuel Grumbach, Luca Coelho,
Intel Linux Wireless, Kalle Valo
Cc: linux-wireless, netdev, linux-kernel, Nick Desaulniers,
Nathan Chancellor
Clang warns:
drivers/net/wireless/intel/iwlwifi/mvm/ftm-initiator.c:465:2: warning:
comparison of distinct pointer types ('typeof ((rtt_avg)) *' (aka 'long
long *') and 'uint64_t *' (aka 'unsigned long long *'))
[-Wcompare-distinct-pointer-types]
do_div(rtt_avg, 6666);
^~~~~~~~~~~~~~~~~~~~~
include/asm-generic/div64.h:222:28: note: expanded from macro 'do_div'
(void)(((typeof((n)) *)0) == ((uint64_t *)0)); \
~~~~~~~~~~~~~~~~~~ ^ ~~~~~~~~~~~~~~~
1 warning generated.
do_div expects an unsigned dividend. Use div64_s64, which expects a
signed dividend.
Fixes: 937b10c0de68 ("iwlwifi: mvm: add debug prints for FTM")
Link: https://github.com/ClangBuiltLinux/linux/issues/372
Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
---
drivers/net/wireless/intel/iwlwifi/mvm/ftm-initiator.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/wireless/intel/iwlwifi/mvm/ftm-initiator.c b/drivers/net/wireless/intel/iwlwifi/mvm/ftm-initiator.c
index e9822a3ec373..92b22250eb7d 100644
--- a/drivers/net/wireless/intel/iwlwifi/mvm/ftm-initiator.c
+++ b/drivers/net/wireless/intel/iwlwifi/mvm/ftm-initiator.c
@@ -462,7 +462,7 @@ static void iwl_mvm_debug_range_resp(struct iwl_mvm *mvm, u8 index,
{
s64 rtt_avg = res->ftm.rtt_avg * 100;
- do_div(rtt_avg, 6666);
+ div64_s64(rtt_avg, 6666);
IWL_DEBUG_INFO(mvm, "entry %d\n", index);
IWL_DEBUG_INFO(mvm, "\tstatus: %d\n", res->status);
--
2.21.0.rc1
^ permalink raw reply related
* Re: [PATCH bpf-next 7/9] bpf: Sample NRM BPF program to limit egress bw
From: Eric Dumazet @ 2019-02-19 18:29 UTC (permalink / raw)
To: brakmo, netdev; +Cc: Martin Lau, Alexei Starovoitov, Daniel Borkmann
In-Reply-To: <20190219053837.2086945-1-brakmo@fb.com>
On 02/18/2019 09:38 PM, brakmo wrote:
> +
> +static __always_inline void get_nrm_pkt_info(struct bpf_sock *sk,
> + struct nrm_pkt_info *pkti)
> +{
> + if (sk->family == AF_INET6 || sk->family == AF_INET) {
> + pkti->is_ip = true;
> + pkti->is_tcp = (sk->protocol == IPPROTO_TCP);
> + if (pkti->is_tcp) {
> + struct bpf_tcp_sock *tp;
> +
> + tp = bpf_tcp_sock(sk);
> + if (tp)
> + pkti->ecn = tp->ecn_flags & TCP_ECN_OK;
> + else
> + pkti->ecn = 0;
> + } else {
> + pkti->ecn = 0;
> + }
> + } else {
> + pkti->is_ip = false;
> + pkti->is_tcp = false;
> + pkti->ecn = 0;
> + }
> +}
>
This looks very strange.
ECN capability is per packet, and does not need access to the original
TCP socket really.
We definitely can use ECN with UDP packets.
IMO this sample looks like a work in progress.
EDT model allows to implement full shaping (not only virtual one)
by twaking/advancing skb->tstamp
(see commit f11216b24219a bpf: add skb->tstamp r/w access from tc clsact and cg skb progs)
Implementing shaping with the need of accessing TCP sockets seems a layering violation,
a lot of things can go wrong with this model.
For instance, you wont be able to offload this.
^ permalink raw reply
* Re: [PATCH bpf-next 3/9] bpf: add bpf helper bpf_skb_set_ecn
From: Eric Dumazet @ 2019-02-19 18:30 UTC (permalink / raw)
To: brakmo, netdev; +Cc: Martin Lau, Alexei Starovoitov
In-Reply-To: <20190219053832.2086706-1-brakmo@fb.com>
On 02/18/2019 09:38 PM, brakmo wrote:
> This patch adds a new bpf helper BPF_FUNC_skb_set_ecn
> "int bpf_skb_set_Ecn(struct sk_buff *skb)". It is added to
> BPF_PROG_TYPE_CGROUP_SKB typed bpf_prog which currently can
> be attached to the ingress and egress path. This type of
> bpf_prog cannot modify the skb directly.
>
> This helper is used to set the ECN bits (2) of the IPv6 or IPv4
> header in skb. It can be used by a bpf_prog to manage egress
> network bandwdith limit per cgroupv2 by inducing an ECN
> response in the TCP sender (when the packet is ECN enabled).
> This works best when using DCTCP.
> +
> +BPF_CALL_2(bpf_skb_set_ecn, struct sk_buff *, skb, u32, val)
> +{
> + struct ipv6hdr *ip6h = ipv6_hdr(skb);
> +
> + if ((val & ~0x3) != 0)
> + return -EINVAL;
> +
> + if (ip6h->version == 6) {
> + ip6h->flow_lbl[0] = (ip6h->flow_lbl[0] & ~0x30) | (val << 4);
> + return 0;
> + } else if (ip6h->version == 4) {
> + struct iphdr *ip4h = (struct iphdr *)ip6h;
> +
> + ip4h->tos = (ip4h->tos & ~0x3) | val;
Why is not the IPv4 checksum recomputed here ?
If you leave this task to the caller, this should be documented.
These hard coded constants are not really nice.
Why not simply using INET_ECN_set_ce() which is IPv4/IPv6 ready ?
Do you really need to set anything else than CE ?
^ permalink raw reply
* Re: [PATCH bpf-next 1/9] bpf: Add bpf helper bpf_tcp_enter_cwr
From: Eric Dumazet @ 2019-02-19 18:30 UTC (permalink / raw)
To: brakmo, netdev; +Cc: Martin Lau, Alexei Starovoitov
In-Reply-To: <20190219053830.2086578-1-brakmo@fb.com>
On 02/18/2019 09:38 PM, brakmo wrote:
> This patch adds a new bpf helper BPF_FUNC_tcp_enter_cwr
> "int bpf_tcp_enter_cwr(struct bpf_tcp_sock *tp)".
> It is added to BPF_PROG_TYPE_CGROUP_SKB typed bpf_prog
> which currently can be attached to the ingress and egress
> path.
>
Do we have the guarantee socket is a tcp one, and that the caller
owns the socket lock ?
Please describe the exact context for this helper being used.
^ 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