Re: [PATCH 1/7] net/dsa: configure autoneg for CPU port

From: Robert Beckett <bob.beckett@collabora.com>
To: Florian Fainelli <f.fainelli@gmail.com>, Andrew Lunn <andrew@lunn.ch>
Cc: netdev@vger.kernel.org, Vivien Didelot <vivien.didelot@gmail.com>,
	"David S. Miller" <davem@davemloft.net>
Subject: Re: [PATCH 1/7] net/dsa: configure autoneg for CPU port
Date: Wed, 11 Sep 2019 10:54:13 +0100	[thread overview]
Message-ID: <6e73ba7cf18f06b39b6a999d09ad71c1aeff2d5b.camel@collabora.com> (raw)
In-Reply-To: <ad302835a98ca5abc7ac88b3caad64867e33ee70.camel@collabora.com>

On Wed, 2019-09-11 at 10:16 +0100, Robert Beckett wrote:
> On Tue, 2019-09-10 at 11:29 -0700, Florian Fainelli wrote:
> > On 9/10/19 11:26 AM, Andrew Lunn wrote:
> > > On Tue, Sep 10, 2019 at 04:41:47PM +0100, Robert Beckett wrote:
> > > > This enables us to negoatiate pause frame transmission to
> > > > prioritise
> > > > packet delivery over throughput.
> > > 
> > > I don't think we can unconditionally enable this. It is a big
> > > behaviour change, and it is likely to break running systems. It
> > > has
> > > affects on QoS, packet prioritisation, etc.
> > > 
> > > I think there needs to be a configuration knob. But
> > > unfortunately,
> > > i
> > > don't know of a good place to put this knob. The switch CPU port
> > > is
> > > not visible in any way.
> > 
> > Broadcast storm suppression is to be solved at ingress, not on the
> > CPU
> > port, once this lands on the CPU port, it's game over already.
> 
> It is not just for broadcast storm protection. The original issue
> that
> made me look in to all of this turned out to be rx descritor ring
> buffer exhaustion due to the CPU not being able to keep up with
> packet
> reception.
> 
> Although the simple repro case for it is a broadcast storm, this
> could
> happen with many legitimate small packets, and the correct way to
> handle it seems to be pause frames, though I am not traditionally a
> network programmer, so my knowledge may be incorrect. Please advise
> if
> you know of a better way to handle that.
> 
> Fundamentally, with a phy to phy CPU connection, the CPU MAC may well
> wish to enable pause frames for various reasons, so we should strive
> to
> handle that I think.
> 

As an aside, do any of you have experience of trying to enable PIRL on
the Marvell switches? The first thing I tried was configuring it for
packet number based (rather than byte count based) input rate limiting,
but it never seemed to have any effect even at extreme values that
should in theory have greatly limited the number of packets allowed to
ingress.

After investigating the root cause and finding it was due to the CPU's
inability to process the received packets quickly enough, pause frames
and port prioritization seemed like the correct fix anyway.