Date:   Wed, 28 Mar 2018 14:53:48 -0700
From:   Florian Fainelli <f.fainelli@...il.com>
To:     Tal Gilboa <talgi@...lanox.com>, netdev@...r.kernel.org
Cc:     davem@...emloft.net, jaedon.shin@...il.com, pgynther@...gle.com,
        opendmb@...il.com, Michael Chan <michael.chan@...adcom.com>,
        gospo@...adcom.com, saeedm@...lanox.com
Subject: Re: [PATCH net-next 2/3] net: systemport: Fix coalescing settings
 handling

On 03/28/2018 02:23 PM, Tal Gilboa wrote:
> On 3/27/2018 10:47 PM, Florian Fainelli wrote:
>> There were a number of issues with setting the RX coalescing parameters:
>>
>> - we would not be preserving values that would have been configured
>>    across close/open calls, instead we would always reset to no timeout
>>    and 1 interrupt per packet, this would also prevent DIM from
>> setting its
>>    default usec/pkts values
>>
>> - when adaptive RX would be turned on, we woud not be fetching the
>>    default parameters, we would stay with no timeout/1 packet per
>>    interrupt until the estimator kicks in and changes that
>>
>> - finally disabling adaptive RX coalescing while providing parameters
>>    would not be honored, and we would stay with whatever DIM had
>>    previously determined instead of the user requested parameters
>>
>> Fixes: b6e0e875421e ("net: systemport: Implement adaptive interrupt
>> coalescing")
>> Signed-off-by: Florian Fainelli <f.fainelli@...il.com>
>> ---
>>   drivers/net/ethernet/broadcom/bcmsysport.c | 57
>> ++++++++++++++++++++----------
>>   drivers/net/ethernet/broadcom/bcmsysport.h |  4 +--
>>   2 files changed, 41 insertions(+), 20 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/broadcom/bcmsysport.c
>> b/drivers/net/ethernet/broadcom/bcmsysport.c
>> index 1e52bb7d822e..43ad6300c351 100644
>> --- a/drivers/net/ethernet/broadcom/bcmsysport.c
>> +++ b/drivers/net/ethernet/broadcom/bcmsysport.c
>> @@ -574,16 +574,16 @@ static int bcm_sysport_set_wol(struct net_device
>> *dev,
>>       return 0;
>>   }
>>   -static void bcm_sysport_set_rx_coalesce(struct bcm_sysport_priv *priv)
>> +static void bcm_sysport_set_rx_coalesce(struct bcm_sysport_priv *priv,
>> +                    u32 usecs, u32 pkts)
>>   {
>>       u32 reg;
>>         reg = rdma_readl(priv, RDMA_MBDONE_INTR);
>>       reg &= ~(RDMA_INTR_THRESH_MASK |
>>            RDMA_TIMEOUT_MASK << RDMA_TIMEOUT_SHIFT);
>> -    reg |= priv->dim.coal_pkts;
>> -    reg |= DIV_ROUND_UP(priv->dim.coal_usecs * 1000, 8192) <<
>> -                RDMA_TIMEOUT_SHIFT;
>> +    reg |= pkts;
>> +    reg |= DIV_ROUND_UP(usecs * 1000, 8192) << RDMA_TIMEOUT_SHIFT;
>>       rdma_writel(priv, reg, RDMA_MBDONE_INTR);
>>   }
>>   @@ -626,6 +626,8 @@ static int bcm_sysport_set_coalesce(struct
>> net_device *dev,
>>                       struct ethtool_coalesce *ec)
>>   {
>>       struct bcm_sysport_priv *priv = netdev_priv(dev);
>> +    struct net_dim_cq_moder moder;
>> +    u32 usecs, pkts;
>>       unsigned int i;
>>         /* Base system clock is 125Mhz, DMA timeout is this reference
>> clock
>> @@ -646,15 +648,22 @@ static int bcm_sysport_set_coalesce(struct
>> net_device *dev,
>>       for (i = 0; i < dev->num_tx_queues; i++)
>>           bcm_sysport_set_tx_coalesce(&priv->tx_rings[i], ec);
>>   -    priv->dim.coal_usecs = ec->rx_coalesce_usecs;
>> -    priv->dim.coal_pkts = ec->rx_max_coalesced_frames;
>> +    priv->rx_coalesce_usecs = ec->rx_coalesce_usecs;
>> +    priv->rx_max_coalesced_frames = ec->rx_max_coalesced_frames;
>> +    usecs = priv->rx_coalesce_usecs;
>> +    pkts = priv->rx_max_coalesced_frames;
>>   -    if (!ec->use_adaptive_rx_coalesce && priv->dim.use_dim) {
>> -        priv->dim.coal_pkts = 1;
>> -        priv->dim.coal_usecs = 0;
>> +    /* If DIM is enabled, immediately obtain default parameters */
>> +    if (ec->use_adaptive_rx_coalesce) {
>> +        moder = net_dim_get_def_profile(priv->dim.dim.mode);
>> +        usecs = moder.usec;
>> +        pkts = moder.pkts;
>>       }
> 
> What if DIM is already in use? We don't want to set default values if
> DIM is already working.

Indeed, good catch, thank you!

> 
>> +
>>       priv->dim.use_dim = ec->use_adaptive_rx_coalesce;
>> -    bcm_sysport_set_rx_coalesce(priv);
>> +
>> +    /* Apply desired coalescing parameters */
>> +    bcm_sysport_set_rx_coalesce(priv, usecs, pkts);
>>         return 0;
>>   }
>> @@ -1058,10 +1067,7 @@ static void bcm_sysport_dim_work(struct
>> work_struct *work)
>>       struct net_dim_cq_moder cur_profile =
>>                   net_dim_get_profile(dim->mode, dim->profile_ix);
>>   -    priv->dim.coal_usecs = cur_profile.usec;
>> -    priv->dim.coal_pkts = cur_profile.pkts;
>> -
>> -    bcm_sysport_set_rx_coalesce(priv);
>> +    bcm_sysport_set_rx_coalesce(priv, cur_profile.usec,
>> cur_profile.pkts);
>>       dim->state = NET_DIM_START_MEASURE;
>>   }
>>   @@ -1408,14 +1414,30 @@ static void bcm_sysport_adj_link(struct
>> net_device *dev)
>>           phy_print_status(phydev);
>>   }
>>   -static void bcm_sysport_init_dim(struct bcm_sysport_net_dim *dim,
>> +static void bcm_sysport_init_dim(struct bcm_sysport_priv *priv,
>>                    void (*cb)(struct work_struct *work))
>>   {
>> +    struct bcm_sysport_net_dim *dim = &priv->dim;
>> +    struct net_dim_cq_moder moder;
>> +    u32 usecs, pkts;
>> +
>>       INIT_WORK(&dim->dim.work, cb);
>>       dim->dim.mode = NET_DIM_CQ_PERIOD_MODE_START_FROM_EQE;
>>       dim->event_ctr = 0;
>>       dim->packets = 0;
>>       dim->bytes = 0;
> 
> I would expect only dim related code in a function called init_dim(). I
> think the code below should be elsewhere. Or maybe change the function
> name to init_rx_coalesce().

Sure, I can definitively do split the initialization, that makes sense.
-- 
Florian

Powered by blists - more mailing lists