Re: [PATCH v2 net-next] net: dsa: felix: enable cut-through forwarding between ports by default

[Vladimir Oltean <olteanv@gmail.com>]

On Wed, Nov 24, 2021 at 06:39:00PM -0800, Jakub Kicinski wrote:
> On Tue, 23 Nov 2021 15:21:16 +0200 Vladimir Oltean wrote:
> > +static void vsc9959_cut_through_fwd(struct ocelot *ocelot)
> > +{
> > +	struct felix *felix = ocelot_to_felix(ocelot);
> > +	struct dsa_switch *ds = felix->ds;
> > +	int port, other_port;
> > +
> > +	for (port = 0; port < ocelot->num_phys_ports; port++) {
> > +		struct ocelot_port *ocelot_port = ocelot->ports[port];
> > +		unsigned long mask;
> > +		int min_speed;
> > +		u32 val = 0;
> > +
> > +		if (ocelot_port->speed <= 0)
> > +			continue;
> 
> Would it not be safer to disable cut-thru for ports which are down?
> 
> 	goto set;

I don't know, I suppose we can do that.

> > +		min_speed = ocelot_port->speed;
> > +		if (port == ocelot->npi) {
> > +			/* Ocelot switches forward from the NPI port towards
> > +			 * any port, regardless of it being in the NPI port's
> > +			 * forwarding domain or not.
> > +			 */
> > +			mask = dsa_user_ports(ds);
> > +		} else {
> > +			mask = ocelot_read_rix(ocelot, ANA_PGID_PGID,
> > +					       PGID_SRC + port);
> > +			/* Ocelot switches forward to the NPI port despite it
> > +			 * not being in the source ports' forwarding domain.
> > +			 */
> > +			if (ocelot->npi >= 0)
> > +				mask |= BIT(ocelot->npi);
> > +		}
> > +
> > +		for_each_set_bit(other_port, &mask, ocelot->num_phys_ports) {
> > +			struct ocelot_port *other_ocelot_port;
> > +
> > +			other_ocelot_port = ocelot->ports[other_port];
> > +			if (other_ocelot_port->speed <= 0)
> > +				continue;
> > +
> > +			if (min_speed > other_ocelot_port->speed)
> > +				min_speed = other_ocelot_port->speed;
> 
> break; ?

Break where and why?
Breaking in the "if" block means "stop at the first @other_port in
@port's forwarding domain which has a lower speed than @port". But that
isn't necessarily the minimum...
And breaking below the "if" block means stopping at the first
@other_port in @port's forwarding domain, which doesn't make sense.
This is the simple calculation of the minimum value of an array, no
special sauce here.

> > +		}
> > +
> > +		/* Enable cut-through forwarding for all traffic classes. */
> > +		if (ocelot_port->speed == min_speed)
> 
> Any particular reason this is not <= ?

So min_speed is by construction always a valid speed: SPEED_10,
SPEED_100 etc (never SPEED_UNKNOWN). What this statement is saying is
that cut-through forwarding can be enabled only for the ports operating
at the lowest link speed within a forwarding domain. If I change "=="
into "<=", I would also be enabling cut-through for the ports with
SPEED_UNKNOWN (were it not for this condition right at the beginning):

		if (ocelot_port->speed <= 0)
			continue;

So practically speaking, since there is never any port with a speed
smaller than the min_speed, it doesn't make sense to check for <= min_speed.

> > +			val = GENMASK(7, 0);
> 
> set: ?

This I can do.

> > +		ocelot_write_rix(ocelot, val, ANA_CUT_THRU_CFG, port);
> > +	}
> > +}
> 
> >  static const struct felix_info felix_info_vsc9959 = {
> > diff --git a/drivers/net/ethernet/mscc/ocelot.c b/drivers/net/ethernet/mscc/ocelot.c
> > index 95920668feb0..30c790f401be 100644
> > --- a/drivers/net/ethernet/mscc/ocelot.c
> > +++ b/drivers/net/ethernet/mscc/ocelot.c
> 
> > @@ -697,6 +702,8 @@ void ocelot_phylink_mac_link_up(struct ocelot *ocelot, int port,
> >  	int mac_speed, mode = 0;
> >  	u32 mac_fc_cfg;
> >  
> > +	ocelot_port->speed = speed;
> > +
> >  	/* The MAC might be integrated in systems where the MAC speed is fixed
> >  	 * and it's the PCS who is performing the rate adaptation, so we have
> >  	 * to write "1000Mbps" into the LINK_SPEED field of DEV_CLOCK_CFG
> > @@ -772,6 +779,9 @@ void ocelot_phylink_mac_link_up(struct ocelot *ocelot, int port,
> >  	/* Core: Enable port for frame transfer */
> >  	ocelot_fields_write(ocelot, port,
> >  			    QSYS_SWITCH_PORT_MODE_PORT_ENA, 1);
> 
> Does this enable forwarding? Is there a window here with forwarding
> enabled and old cut-thru masks if we don't clear cut-thru when port
> goes down?

Correct, I should be updating the cut-through masks before this, thanks.

> > +	if (ocelot->ops->cut_through_fwd)
> > +		ocelot->ops->cut_through_fwd(ocelot);
> >  }
> >  EXPORT_SYMBOL_GPL(ocelot_phylink_mac_link_up);
> >  
> > @@ -1637,6 +1647,9 @@ void ocelot_apply_bridge_fwd_mask(struct ocelot *ocelot)
> >  
> >  		ocelot_write_rix(ocelot, mask, ANA_PGID_PGID, PGID_SRC + port);
> >  	}
> 
> Obviously shooting from the hip here, but I was expecting the cut-thru
> update to be before the bridge reconfig if port is joining, and after
> if port is leaving. Do you know what I'm getting at?

Yes, I know what you're getting at. But it's a bit complicated to do,
given the layering constraints and that cut-through forwarding is an
optional feature which isn't present on all devices, so I am trying to
keep its footprint minimal on the ocelot library.

What I can do is I can disable cut-through forwarding for ports that are
standalone (not in a bridge). I don't have a use case for that anyway:
the store-and-forward latency is indistinguishable from network stack
latency. This will guarantee that when a port joins a bridge, it has
cut-through forwarding disabled. So there are no issues if it happens to
join a bridge and its link speed is higher than anybody else: there will
be no packet underruns.

> > +	if (ocelot->ops->cut_through_fwd)
> > +		ocelot->ops->cut_through_fwd(ocelot);
> >  }
> >  EXPORT_SYMBOL(ocelot_apply_bridge_fwd_mask);

Thanks for taking a look at the patch!