From mboxrd@z Thu Jan 1 00:00:00 1970 From: Florian Fainelli Subject: Re: [PATCH net-next 2/3] net: systemport: Fix coalescing settings handling Date: Wed, 28 Mar 2018 14:53:48 -0700 Message-ID: <7ad32c91-088f-4619-329b-62bdedae8507@gmail.com> References: <20180327194707.31857-1-f.fainelli@gmail.com> <20180327194707.31857-3-f.fainelli@gmail.com> <61c5d8c2-5bc8-9f61-97aa-fb42eceb753f@mellanox.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Cc: davem@davemloft.net, jaedon.shin@gmail.com, pgynther@google.com, opendmb@gmail.com, Michael Chan , gospo@broadcom.com, saeedm@mellanox.com To: Tal Gilboa , netdev@vger.kernel.org Return-path: Received: from mail-wm0-f65.google.com ([74.125.82.65]:53587 "EHLO mail-wm0-f65.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753444AbeC1Vx6 (ORCPT ); Wed, 28 Mar 2018 17:53:58 -0400 Received: by mail-wm0-f65.google.com with SMTP id p9so7405483wmc.3 for ; Wed, 28 Mar 2018 14:53:58 -0700 (PDT) In-Reply-To: <61c5d8c2-5bc8-9f61-97aa-fb42eceb753f@mellanox.com> Content-Language: en-US Sender: netdev-owner@vger.kernel.org List-ID: 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 >> --- >>   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