Date:   Fri, 24 Nov 2017 04:20:17 -0800
From:   Saeed Mahameed <saeedm@....mellanox.co.il>
To:     Andy Gospodarek <andy@...yhouse.net>,
        Tariq Toukan <tariqt@...lanox.com>, talgi@...lanox.com
Cc:     Linux Netdev List <netdev@...r.kernel.org>,
        Michael Chan <mchan@...adcom.com>,
        Saeed Mahameed <saeedm@...lanox.com>,
        Andy Gospodarek <gospo@...adcom.com>
Subject: Re: [RFC 0/9] net: create adaptive software irq moderation library

On Fri, Nov 24, 2017 at 4:05 AM, Saeed Mahameed
<saeedm@....mellanox.co.il> wrote:
> On Sun, Nov 5, 2017 at 9:44 PM, Andy Gospodarek <andy@...yhouse.net> wrote:
>> From: Andy Gospodarek <gospo@...adcom.com>
>>
>> This RFC converts the adaptive interrupt moderation library from the
>> mlx5_en driver into a library so it can be used by any driver.  The last
>> patch in this set adds support for interrupt moderation in the bnxt_en
>> driver.
>>
>> The main purpose of this code in the mlx5_en driver is to allow an
>> administrator to make sure that default coalesce settings are optimized
>> for low latency, but quickly adapt to handle high throughput traffic and
>> optimize how many packets are received during each napi poll.
>>
>> For any new driver the following changes would be needed to use this
>> library:
>>
>> - add elements in ring struct to track items needed by this library
>> - create function that can be called to actually set coalesce settings
>>   for the driver
>>
>> My main reason for making this an RFC is that I would like verification
>> from Mellanox that the performance of their driver does not change in a
>> unintended way.  I did some basic testing (netperf) and did not note a
>> statistically significant change in throughput or CPU utilization before
>> and after this set.
>>
>> Credit to Rob Rice and Lee Reed for doing some of the initial proof of
>> concept and testing for this patch.
>
> Hi Andy,
>
> Following our conversation in netdev 2.2,  i would like to suggest the
> following:
>
> Instead of introducing a new API which demands from the driver to
> provide callbacks and function pointers to the adaptive moderation
> logic, which might be called on every irq interrupt, and to avoid
> performance hit, we can move the generic code and the core adaptive
> moderation logic to a header file.
>

I would like also to suggesting adding Tal Gilboa, as the official
maintainer for this new file.
as he is the current maintainer and the co-author of this feature in mlx5.

> the mlx5e am logic and data structures are already written in a very
> modular way and can be stripped out of mlx5e fairly easily.
> And i would like to suggest to do it in the following manner:
>
> 1. naming convention:
> I would like to change the generic code naming convention to have the
> words DIM (Dynamically-Tuned Interrupt Moderation) instead of mlx5e_am
> or am, Following our public blog [1] of the matter and the official
> name we prefer for this feature.
>
> [1] https://community.mellanox.com/docs/DOC-2511
>
> Suggested naming convention instead of rx_am:  net_dim (DIM for net
> applications).
> As the rx_am or (dim) logic can be applied to other applications.
>
> 2. Data types:
>
> All below mlx5e am data types can be used as is as they hold nothing
> mlx5 related.
>
> struct mlx5e_rx_am_sample
>   - Holds the current stats sample with ktime stamp
>   - rename to: net_dim_sample
>
> struct mlx5e_rx_am_stats
>  - Holds the needed stats (delta) calculation of last 2 samples
>  - rename to: net_dim_stats
>
> struct mlx5e_rx_am
>  - Adaptive moderation handle
>  - rename to: net_dim
>
> 3. static inline generic functions API (based on the usage from
> mlx5e_rx_am function)
>
> //Make a DIM measurement:
> net_dim_sample(struct *net_dim_sample sample, packets, bytes, event_ctr)
> - previously mlx5e_am_sample()
> - Fills a sample struct with the provided stats and the current timestamp
>
>
> //start a new DIM measurement and handles the DIM state machine initial state:
> net_dim_start_sample(struct *net_dim rx_dim)
>  - Makes a new measurement
>  - stores it into rx_dim->start
>  - rx_dim->state = DIM_MEASURE_IN_PROGRESS
>
>
> // Takes a new sample (curr_sample) and makes the decision (handles
> DIM_MEASURE_IN_PROGRESS state)
> net_dim_decision(struct *net_dim rx_dim, curr_sample)
>   -  previously mlx5e_am_decision
>   - Note, instead of providing the current_stats (delta between start
> and current_sample) I suggest to provide the current_sample and move
> the stats calculation logic into net_dime_decision.
>    - All the logic in this function will move to the generic code.
>
> 4. Driver implementation: (according to the above suggested API)
>    -  Driver should initialize struct net_dim rx_dim, and provide a
> work function to handle "dim apply new profile" decision.
>    - in napi_poll driver should implement the rx_dim state machine
> using the above API before arming the completion event queues as
> follows:
>
> mlx5e_rx_am:
>
> void mlx5e_rx_am(struct mlx5e_rq *rq)
> {
>        struct net_dim *rx_dim = &rq->dim;
>        struct net_dim_sample end_sample;
>        u16 nevents;
>
>        switch (rx_dim->state) {
>        case DIM_MEASURE_IN_PROGRESS:
>            // driver specific pre condition to decide whether to
> continue or skip
>            // Note that here we only sample and don't calc the delta
> stats, this logic moved into net_dim_decision
>            net_dim_sample(rq, &end_sample, rq->packets, rq->bytes, cq->events);
>            if (net_dim_decision(rx_dim, &end_sample)) {
>                rx_dim->state = DIM_APPLY_NEW_PROFILE;
>                schedule_work(&rx_dim->work);
>             }
>         /* fall through */
>        case DIM_START_MEASURE:
>            net_dim_start_sample(rx_dim);
>        break;
>        case DIM_APPLY_NEW_PROFILE:
>        break;
> }
>
> Thanks,
> Saeed.
>

Powered by blists - more mailing lists