Re: [PATCH net-next 1/3] net: dsa: Configure the MTU for switch ports

[Vladimir Oltean <olteanv@gmail.com>]

On Sat, 23 Nov 2019 at 22:28, Florian Fainelli <f.fainelli@gmail.com> wrote:
>
> Hi Vladimir,
>
> On 11/23/2019 11:48 AM, Vladimir Oltean wrote:
> > It is useful be able to configure port policers on a switch to accept
> > frames of various sizes:
> >
> > - Increase the MTU for better throughput from the default of 1500 if it
> >   is known that there is no 10/100 Mbps device in the network.
> > - Decrease the MTU to limit the latency of high-priority frames under
> >   congestion.
> >
> > For DSA slave ports, this is mostly a pass-through callback, called
> > through the regular ndo ops and at probe time (to ensure consistency
> > across all supported switches).
> >
> > The CPU port is called with an MTU equal to the largest configured MTU
> > of the slave ports. The assumption is that the user might want to
> > sustain a bidirectional conversation with a partner over any switch
> > port.
> >
> > The DSA master is configured the same as the CPU port, plus the tagger
> > overhead. Since the MTU is by definition L2 payload (sans Ethernet
> > header), it is up to each individual driver to figure out if it needs to
> > do anything special for its frame tags on the CPU port (it shouldn't
> > except in special cases). So the MTU does not contain the tagger
> > overhead on the CPU port.
> > However the MTU of the DSA master, minus the tagger overhead, is used as
> > a proxy for the MTU of the CPU port, which does not have a net device.
> > This is to avoid uselessly calling the .change_mtu function on the CPU
> > port when nothing should change.
> >
> > So it is safe to assume that the DSA master and the CPU port MTUs are
> > apart by exactly the tagger's overhead in bytes.
> >
> > Signed-off-by: Vladimir Oltean <olteanv@gmail.com>
> > ---
>
> [snip]
> > +static int dsa_slave_change_mtu(struct net_device *dev, int new_mtu)
> > +{
> > +     struct net_device *master = dsa_slave_to_master(dev);
> > +     struct dsa_slave_priv *p = netdev_priv(dev);
> > +     struct dsa_switch *ds = p->dp->ds;
> > +     struct dsa_port *cpu_dp;
> > +     int port = p->dp->index;
> > +     int max_mtu = 0;
> > +     int cpu_mtu;
> > +     int err, i;
> > +
> > +     if (!ds->ops->change_mtu)
> > +             return -EOPNOTSUPP;
> > +
> > +     err = ds->ops->change_mtu(ds, port, new_mtu);
> > +     if (err < 0)
> > +             return err;
> > +
> > +     dev->mtu = new_mtu;
> > +
> > +     for (i = 0; i < ds->num_ports; i++) {
> > +             if (!dsa_is_user_port(ds, i))
> > +                     continue;
> > +
> > +             /* During probe, this function will be called for each slave
> > +              * device, while not all of them have been allocated. That's
> > +              * ok, it doesn't change what the maximum is, so ignore it.
> > +              */
> > +             if (!dsa_to_port(ds, i)->slave)
> > +                     continue;
> > +
> > +             if (max_mtu < dsa_to_port(ds, i)->slave->mtu)
> > +                     max_mtu = dsa_to_port(ds, i)->slave->mtu;
> > +     }
> > +
> > +     cpu_dp = dsa_to_port(ds, port)->cpu_dp;
> > +
> > +     max_mtu += cpu_dp->tag_ops->overhead;
> > +     cpu_mtu = master->mtu;
> > +
> > +     if (max_mtu != cpu_mtu) {
> > +             err = ds->ops->change_mtu(ds, dsa_upstream_port(ds, port),
> > +                                       max_mtu - cpu_dp->tag_ops->overhead);
> > +             if (err < 0)
> > +                     return err;
>
> Before changing and committing the slave_dev's MTU you should actually
> perform these two operations first to make sure that you can honor the
> user port MTU that is requested. Here, you would possibly leave an user
> port configured for a MTU value that is unsupported by the upstream
> port(s) and/or the CPU port and/or the DSA master device, which could
> possibly break frame forwarding depending on what the switch is willing
> to accept.
>

Correct. I was actually held back a bit while looking at Andrew's
patch dc0fe7d47f9f ("net: dsa: Set the master device's MTU to account
for DSA overheads") where he basically discarded errors, so that's the
approach I took too (thinking that some DSA masters would not have ops
for changing or reporting the MTU).

> I had prepared a patch series with Murali doing nearly the same thing
> and targeting Broadcom switches nearly a year ago but since I never got
> feedback whether this worked properly for the use case he was after, I
> did not submit it since I did not need it personally and found it to be
> a nice can of worms.
>

Nice, do you mind if I take your series instead then?

> Another thing that I had not gotten around testing was making sure that
> when a slave_dev gets enslaved as a bridge port member, that bridge MTU
> normalization would kick in and make sure that if you have say: port 0
> configured with MTU 1500 and port 1 configured with MTU 9000, the bridge
> would normalize to MTU 1500 as you would expect.
>

Nope, that doesn't happen by default, at least in my implementation.
Is there code in the bridge core for it?

> https://github.com/ffainelli/linux/commits/dsa-mtu
>
> This should be a DSA switch fabric notifier IMHO because changing the
> MTU on an user port implies changing the MTU on every DSA port in
> between plus the CPU port. Your approach here works for the first
> upstream port, but not for the ones in between, and there can be more,
> as is common with the ZII devel Rev. B and C boards.

Yes, correct. Your patch implements notifiers which is definitely
good. I don't have a cascaded setup to test yet (my Turris Mox was
supposed to arrive but for some reason it was returned to the seller
by the shipping company...).

> --
> Florian

Thanks,
-Vladimir