Re: [RFC PATCH net-next 0/2] Configuring PFC stall prevention via ethtool

[Eran Ben Elisha]

On Thu, Nov 16, 2017 at 5:42 PM, Andrew Lunn  wrote:
>> I don't like adding another ethtool_ops callback tightly tied to the
>> structures passed via ioctl() but when I started to think what to
>> suggest as an alternative, I started to wonder if it is really necessary
>> to add a new ethtool command at all. Couldn't this be handled as
>> a tunable?
>
> I agree with Michal here.
>
> And as he pointed out, there does not need to be a 1:1 mapping between
> ethtool(1) and the kAPI. I suggest extending the existing -a option,
> and have it make two system calls if needed.
>
> Andrew

Sound good to me. We will follow this suggestion to extend -a using
the tunable op.
In addition, we will come up with new API to use timeouts  and on/off
instead of auto/default.

Eran

Re: [RFC PATCH net-next 0/2] Configuring PFC stall prevention via ethtool

[Andrew Lunn]

> I don't like adding another ethtool_ops callback tightly tied to the
> structures passed via ioctl() but when I started to think what to
> suggest as an alternative, I started to wonder if it is really necessary
> to add a new ethtool command at all. Couldn't this be handled as
> a tunable?

I agree with Michal here.

And as he pointed out, there does not need to be a 1:1 mapping between
ethtool(1) and the kAPI. I suggest extending the existing -a option,
and have it make two system calls if needed.

Andrew

Re: [RFC PATCH net-next 0/2] Configuring PFC stall prevention via ethtool

[Andrew Lunn]

On Thu, Nov 16, 2017 at 11:17:36AM +0200, Eran Ben Elisha wrote:
> On Thu, Nov 16, 2017 at 4:44 AM, Andrew Lunn  wrote:
> >> What do other vendors support? Time? Number of pause frames sent?
> >
> > So i checked a few Marvell Switches. You can also specify a time. It
> > is a little bit more complex than that, since the units of time depend
> > on the link speed. But converting a time in ms to what the register
> > wants is possible.
> >
> > So i'm thinking rather than a poorly defined 'Auto', passing a time
> > would be better.
> >
> >   Andrew
> Hi Andrew,
> 
> We were using the term 'Auto' for few reasons.
> 1. Not confusing the user with the question of what is the correct
> value (100 ms is good? Bad?)
> 2. Allowing exposure of new mechanism in the future without user need
> to change its commands
> 3. Letting the device to decide on best approach according to its
> capabilities, link speed, etc.
> 
> Our initial thought was to expose with timeout as you suggested, but
> it felt very restrictive due to the reasons I mentioned.

I just find 'auto' to be very unclearly defined. Auto-negotiation is
well defined, it is specified in 802.3. But what does Auto mean here?
Why 8ms? Why not 42ms? or 420ms? Auto also generally means some sort
of dynamic behaviour. Make changes depending on the current
conditions. Where as your implementation seems to be fixed at 8ms.

Does 802.3 say anything about this at all? Does it list the 8 seconds
your driver defaults to?

Thanks
Andrew

Re: [RFC PATCH net-next 0/2] Configuring PFC stall prevention via ethtool

[Michal Kubecek]

On Thu, Nov 16, 2017 at 02:03:21PM +0200, Eran Ben Elisha wrote:
> On Thu, Nov 16, 2017 at 10:44 AM, Michal Kubecek  wrote:
> >
> > I don't like adding another ethtool_ops callback tightly tied to the
> > structures passed via ioctl() but when I started to think what to
> > suggest as an alternative, I started to wonder if it is really necessary
> > to add a new ethtool command at all. Couldn't this be handled as
> > a tunable?
> 
> tunable seems as a good infrastructure to PHY tuning, however this
> feature is not a PHY feature.

There are two kinds: tunables (ETHTOOL_{G,S}TUNABLE) and phy tunables
(ETHTOOL_PHY_{G,S}TUNABLE). My understanding is that former is meant as
a generic interface for parameters related to net_device, latter for
parameters related to phydev.

It's only my guess but IMHO the idea behind (both kinds of) tunables was
to add at least some extensibility to the ioctl() interface so that we
don't have to add more and more new one-purpose commands (until we can
switch to netlink).

> To me, it's looks fit to ethtool -a where pause operations are being
> controlled.  Unfortunately set/get_pauseparam is not extensible and
> need a new operation.

Yes, logically it belongs there, no question about that. But the
relation between ethtool subcommands (on command line) and ioctl
commands (cmd member of the structures) is not 1:1 in general (this is
one of the reasons. After all, your patchset uses another command
anyway; my suggestion is to use an existing one (ETHTOOL_{G,S}TUNABLE)
rather than adding a new one (and new callback to ethtool_ops).

Michal Kubecek

Re: [RFC PATCH net-next 0/2] Configuring PFC stall prevention via ethtool

[Eran Ben Elisha]

On Thu, Nov 16, 2017 at 10:44 AM, Michal Kubecek  wrote:
> On Wed, Nov 15, 2017 at 09:00:09PM +0200, Eran Ben Elisha wrote:
>> From: Inbar Karmy 
>>
>> This RFC adds support for configuring PFC stall prevention through ethtool.
>>
>> In the event where the device unexpectedly becomes unresponsive for a long
>> period of time, flow control mechanism may propagate pause frames which will
>> cause congestion spreading to the entire network.
>>
>> To prevent this scenario, the device may implement a protection mechanism for
>> monitoring and resolving such state.  The following patches allow the user to
>> control the stall prevention functionality.
>>
>> PFC stall prevention configuration is done via ethtool -a (pause).
>> Two modes are introduced:
>> Default - current behavior per driver.
>> Auto - protection mechanism controlled automatically by the driver.
>> Due to lack of extension ability of ethtool_ops set_pauseparam, a new
>> ethtool_ops get_pfc_prevention_mode is introduced.
>
> I don't like adding another ethtool_ops callback tightly tied to the
> structures passed via ioctl() but when I started to think what to
> suggest as an alternative, I started to wonder if it is really necessary
> to add a new ethtool command at all. Couldn't this be handled as
> a tunable?
>
> Michal Kubecek

tunable seems as a good infrastructure to PHY tuning, however this
feature is not a PHY feature.
To me, it's looks fit to ethtool -a where pause operations are being controlled.
Unfortunately set/get_pauseparam is not extensible and need a new operation.

Eran.

Re: [RFC PATCH net-next 0/2] Configuring PFC stall prevention via ethtool

[Eran Ben Elisha]

On Thu, Nov 16, 2017 at 4:44 AM, Andrew Lunn  wrote:
>> What do other vendors support? Time? Number of pause frames sent?
>
> So i checked a few Marvell Switches. You can also specify a time. It
> is a little bit more complex than that, since the units of time depend
> on the link speed. But converting a time in ms to what the register
> wants is possible.
>
> So i'm thinking rather than a poorly defined 'Auto', passing a time
> would be better.
>
>   Andrew
Hi Andrew,

We were using the term 'Auto' for few reasons.
1. Not confusing the user with the question of what is the correct
value (100 ms is good? Bad?)
2. Allowing exposure of new mechanism in the future without user need
to change its commands
3. Letting the device to decide on best approach according to its
capabilities, link speed, etc.

Our initial thought was to expose with timeout as you suggested, but
it felt very restrictive due to the reasons I mentioned.

Eran

Re: [RFC PATCH net-next 0/2] Configuring PFC stall prevention via ethtool

[Michal Kubecek]

On Wed, Nov 15, 2017 at 09:00:09PM +0200, Eran Ben Elisha wrote:
> From: Inbar Karmy 
> 
> This RFC adds support for configuring PFC stall prevention through ethtool.
> 
> In the event where the device unexpectedly becomes unresponsive for a long
> period of time, flow control mechanism may propagate pause frames which will
> cause congestion spreading to the entire network.
> 
> To prevent this scenario, the device may implement a protection mechanism for
> monitoring and resolving such state.  The following patches allow the user to
> control the stall prevention functionality.
> 
> PFC stall prevention configuration is done via ethtool -a (pause).
> Two modes are introduced:
> Default - current behavior per driver.
> Auto - protection mechanism controlled automatically by the driver.
> Due to lack of extension ability of ethtool_ops set_pauseparam, a new
> ethtool_ops get_pfc_prevention_mode is introduced.

I don't like adding another ethtool_ops callback tightly tied to the
structures passed via ioctl() but when I started to think what to
suggest as an alternative, I started to wonder if it is really necessary
to add a new ethtool command at all. Couldn't this be handled as
a tunable?

Michal Kubecek

> I based this patch set on net-next commit 50895b9de1d3("tcp: highest_sack fix
> ").
> 
> Inbar Karmy (2):
>   ethtool: Add support for configuring PFC stall prevention in ethtool
>   net/mlx5e: PFC stall prevention support
> 
>  .../net/ethernet/mellanox/mlx5/core/en_ethtool.c   | 51 
>  drivers/net/ethernet/mellanox/mlx5/core/port.c | 56 
> ++
>  include/linux/ethtool.h|  8 
>  include/linux/mlx5/mlx5_ifc.h  | 22 +++--
>  include/linux/mlx5/port.h  |  5 ++
>  include/uapi/linux/ethtool.h   | 20 
>  net/core/ethtool.c | 39 +++
>  7 files changed, 188 insertions(+), 13 deletions(-)
> 
> -- 
> 1.8.3.1
> 

Re: [RFC PATCH net-next 0/2] Configuring PFC stall prevention via ethtool

[Andrew Lunn]

> What do other vendors support? Time? Number of pause frames sent?

So i checked a few Marvell Switches. You can also specify a time. It
is a little bit more complex than that, since the units of time depend
on the link speed. But converting a time in ms to what the register
wants is possible.

So i'm thinking rather than a poorly defined 'Auto', passing a time
would be better.

  Andrew

Re: [RFC PATCH net-next 0/2] Configuring PFC stall prevention via ethtool

[Andrew Lunn]

On Wed, Nov 15, 2017 at 09:00:09PM +0200, Eran Ben Elisha wrote:
> From: Inbar Karmy 
> 
> This RFC adds support for configuring PFC stall prevention through ethtool.
> 
> In the event where the device unexpectedly becomes unresponsive for a long
> period of time, flow control mechanism may propagate pause frames which will
> cause congestion spreading to the entire network.
> 
> To prevent this scenario, the device may implement a protection mechanism for
> monitoring and resolving such state.  The following patches allow the user to
> control the stall prevention functionality.
> 
> PFC stall prevention configuration is done via ethtool -a (pause).
> Two modes are introduced:
> Default - current behavior per driver.
> Auto - protection mechanism controlled automatically by the driver.

Why Auto?

Down in the driver you seem to translate this to a time. And it looks
like your hardware is flexible on that time, it can probably do at
least 8s to 100ms.

Why not specify a time?

What do other vendors support? Time? Number of pause frames sent?

 Andrew