Date:   Tue, 19 Sep 2023 16:42:27 -0400
From:   "Nabil S. Alramli" <dev@...ramli.com>
To:     Rahul Rameshbabu <rrameshbabu@...dia.com>
Cc:     netdev@...r.kernel.org, saeedm@...dia.com, saeed@...nel.org,
        kuba@...nel.org, davem@...emloft.net, tariqt@...dia.com,
        linux-kernel@...r.kernel.org, leon@...nel.org, jdamato@...tly.com,
        sbhogavilli@...tly.com, nalramli@...tly.com
Subject: Re: [net-next RFC v2 0/4] mlx5: support per queue coalesce settings

Hi Rahul,

Thank you for your response.

On 9/19/23 14:55, Rahul Rameshbabu wrote:
> Hi Nabil,
> 
> On Mon, 18 Sep, 2023 18:29:51 -0400 "Nabil S. Alramli" <dev@...ramli.com> wrote:
>> Hello,
>>
>> This is v2 of my previous patch:
>> https://lore.kernel.org/lkml/20230823223121.58676-1-dev@nalramli.com/.
>>
>> Saeed: Thanks for reviewing v1. I made significant changes to support
>> per-channel DIM settings. Is this ready for an official v1 submission or
>> are there other major changes you'd like to see before I do that?
>>
>> ***************************************************************************
>> Version History
>> ---------------
>> * v1: Initial draft, individual channel DIM changes not supported.
>> * v2: Support individual channel DIM changes.
>> ***************************************************************************
> 
> We actually began working on a patch set for the feature internally
> inspired by your initial RFC. If it is alright with you, would it be ok
> to have you as a co-author of that series that we should have prepared
> in the coming days? We have some minor enhancements that we think will
> improve the general architecture for how we handle both the global and
> per-queue settings.
> 

Yes. Please feel free to add me as a co-author. Actually, I'm new to
submitting mlx-5 patches and a lot of credit goes to Joe Damato
<jdamato@...tly.com> who had this initial idea and helped me develop it
into this patch, so would you mind adding him as well? If you would like
you could start with my patch-set and then revert it and add your own,
or if you think that's too much trouble then I'm fine with however you'd
like to proceed. I'd be happy to test your patch whenever it's ready.

>>
>> Currently, only gobal coalescing configuration queries or changes are
>> supported in the `mlx5` driver. However, per-queue operations are not, and
>> result in `EOPNOTSUPP` errors when attempted with `ethtool`. This patch
>> adds support for per-queue coalesce operations.
>>
>> Here's an example use case:
>>
>> - A mlx5 NIC is configured with 8 queues, each queue has its IRQ pinned to
>>    a unique CPU.
>> - Two custom RSS contexts are created: context 1 and context 2. Each
>>    context has a different set of queues where flows are distributed. For
>>    example, context 1 may distribute flows to queues 0-3, and context 2 may
>>    distribute flows to queues 4-7.
>> - A series of ntuple filters are installed which direct matching flows to
>>    RSS contexts. For example, perhaps port 80 is directed to context 1 and
>>    port 443 to context 2.
>> - Applications which receive network data associated with either context
>>    are pinned to the CPUs where the queues in the matching context have
>>    their IRQs pinned to maximize locality.
>>
>> The apps themselves, however, may have different requirements on latency vs
>> CPU usage and so setting the per queue IRQ coalesce values would be very
>> helpful.
>>
>> This patch would support this. In v1 DIM mode changes could only be changed
>> NIC-wide. However, in this iteration, DIM mode changes are supported
>> globally as well as on a per-queue basis.
>>
>> Here's an example:
>>
>> ```
>> $ sudo ethtool --per-queue eth0 queue_mask 0x4 --show-coalesce
>> Queue: 2
>> Adaptive RX: on  TX: on
>> stats-block-usecs: 0
>> sample-interval: 0
>> pkt-rate-low: 0
>> pkt-rate-high: 0
>>
>> rx-usecs: 8
>> rx-frames: 128
>> rx-usecs-irq: 0
>> rx-frames-irq: 0
>>
>> tx-usecs: 8
>> tx-frames: 128
>> tx-usecs-irq: 0
>> tx-frames-irq: 0
>> ```
>>
>> Now, let's try to set adaptive-rx off rx-usecs 16 for queue 2:
>>
>> ```
>> $ sudo ethtool --per-queue eth0 queue_mask 0x4 --coalesce adaptive-rx off
>> $ sudo ethtool --per-queue eth0 queue_mask 0x4 --coalesce rx-usecs 16
>> ```
>>
>> Confirm that the operation succeeded:
>>
>> ```
>> $ sudo ethtool --per-queue eth0 queue_mask 0x4 --show-coalesce
>> Queue: 2
>> Adaptive RX: off  TX: on
>> stats-block-usecs: 0
>> sample-interval: 0
>> pkt-rate-low: 0
>> pkt-rate-high: 0
>>
>> rx-usecs: 16
>> rx-frames: 32
>> rx-usecs-irq: 0
>> rx-frames-irq: 0
>>
>> tx-usecs: 8
>> tx-frames: 128
>> tx-usecs-irq: 0
>> tx-frames-irq: 0
>> ```
>>
>> The individual channel settings do not overwrite the global ones. However
>> Setting the global parameters will also reset all of the individual channel
>> options. For example, after we set the options for queue 2, we'll see that
>> the global options remain unchanged:
>> ```
>> $ sudo ethtool --show-coalesce eth0
>> Coalesce parameters for eth0:
>> Adaptive RX: on  TX: on
>> stats-block-usecs: 0
>> sample-interval: 0
>> pkt-rate-low: 0
>> pkt-rate-high: 0
>>
>> rx-usecs: 8
>> rx-frames: 128
>> rx-usecs-irq: 0
>> rx-frames-irq: 0
>>
>> tx-usecs: 16
>> tx-frames: 32
>> tx-usecs-irq: 0
>> tx-frames-irq: 0
>> ```
>>
>> But then if we set them, we'll see that the options for queue 2 have been
>> reset as well:
>> ```
>> $ sudo ethtool --coalesce eth0 adaptive-tx off
>>
>> $ sudo ethtool --show-coalesce eth0
>> Coalesce parameters for eth0:
>> Adaptive RX: on  TX: off
>> stats-block-usecs: 0
>> sample-interval: 0
>> pkt-rate-low: 0
>> pkt-rate-high: 0
>>
>> rx-usecs: 8
>> rx-frames: 128
>> rx-usecs-irq: 0
>> rx-frames-irq: 0
>>
>> tx-usecs: 16
>> tx-frames: 32
>> tx-usecs-irq: 0
>> tx-frames-irq: 0
>>
>> $ sudo ethtool --per-queue eth0 queue_mask 0x4 --show-coalesce
>> Queue: 2
>> Adaptive RX: on  TX: off
>> stats-block-usecs: 0
>> sample-interval: 0
>> pkt-rate-low: 0
>> pkt-rate-high: 0
>>
>> rx-usecs: 8
>> rx-frames: 128
>> rx-usecs-irq: 0
>> rx-frames-irq: 0
>>
>> tx-usecs: 16
>> tx-frames: 32
>> tx-usecs-irq: 0
>> tx-frames-irq: 0
>> ```
>>
>> Previously a global `struct mlx5e_params` stored the options in
>> `struct mlx5e_priv.channels.params`. That was preserved, but a channel-
>> specific instance was added as well, in `struct mlx5e_channel.params`.
>>
>> Best Regards,
>>
>> ***************************************************************************
>>
>> Nabil S. Alramli (4):
>>    mlx5: Add mlx5e_param to individual mlx5e_channel and preserve them
>>      through mlx5e_open_channels()
>>    mlx5: Add queue number parameter to mlx5e_safe_switch_params()
> 
> We currently are working on a variation of this without needing to use
> mlx5e_safe_switch_params for updating individual channel states (our
> variation of the feature avoids needing to place an instance of
> mlx5e_params per channel).
> 

Oh I'm curious to see how this solution works. I look forward to your
upcoming patch, and would be happy to review it as well.

>>    mlx5: Implement mlx5e_ethtool_{get,set}_per_queue_coalesce() to
>>      support per-queue operations
>>    mlx5: Add {get,set}_per_queue_coalesce()
>>
>>   drivers/net/ethernet/mellanox/mlx5/core/en.h  |   6 +-
>>   .../ethernet/mellanox/mlx5/core/en_dcbnl.c    |   2 +-
>>   .../ethernet/mellanox/mlx5/core/en_ethtool.c  | 214 +++++++++++++-----
>>   .../net/ethernet/mellanox/mlx5/core/en_main.c |  76 +++++--
>>   .../ethernet/mellanox/mlx5/core/ipoib/ipoib.c |   2 +-
>>   5 files changed, 222 insertions(+), 78 deletions(-)
> 
> --
> Thanks,
> 
> Rahul Rameshbabu

Best Regards,

-- Nabil S. Alramli

Powered by blists - more mailing lists