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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20211125072909.23b4e9d0@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com>
Date:   Thu, 25 Nov 2021 07:29:09 -0800
From:   Jakub Kicinski <kuba@...nel.org>
To:     Vladimir Oltean <olteanv@...il.com>
Cc:     netdev@...r.kernel.org, "David S. Miller" <davem@...emloft.net>,
        Claudiu Manoil <claudiu.manoil@....com>,
        Alexandre Belloni <alexandre.belloni@...tlin.com>,
        UNGLinuxDriver@...rochip.com, Andrew Lunn <andrew@...n.ch>,
        Vivien Didelot <vivien.didelot@...il.com>,
        Florian Fainelli <f.fainelli@...il.com>,
        Xiaoliang Yang <xiaoliang.yang_1@....com>,
        Vladimir Oltean <vladimir.oltean@....com>
Subject: Re: [PATCH v2 net-next] net: dsa: felix: enable cut-through
 forwarding between ports by default

On Thu, 25 Nov 2021 12:16:52 +0200 Vladimir Oltean wrote:
> > > +			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.

A single slower port is enough to disable cut through, this is can be
read as a proof of nonexistence rather than min calculation. But really
just a nit pick, don't think any bot will bother us about it.

> > >  	/* 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.

Hm, to make sure I understand - fixing standalone ports doesn't
necessary address the issue of a slow standalone port joining, right?