| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <3c9df1a7-404c-d5cd-4eda-42ef499b16ae@mellanox.com>
Date: Thu, 29 Mar 2018 10:42:44 +0300
From: Tal Gilboa <talgi@...lanox.com>
To: Florian Fainelli <f.fainelli@...il.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 v2 2/3] net: systemport: Fix coalescing settings
handling
On 3/29/2018 1:15 AM, 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 | 62 +++++++++++++++++++++---------
> drivers/net/ethernet/broadcom/bcmsysport.h | 4 +-
> 2 files changed, 46 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/net/ethernet/broadcom/bcmsysport.c b/drivers/net/ethernet/broadcom/bcmsysport.c
> index 1e52bb7d822e..4a75b1de22e0 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,21 @@ 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 (ec->use_adaptive_rx_coalesce && !priv->dim.use_dim) {
> + moder = net_dim_get_def_profile(priv->dim.dim.mode);
> + usecs = moder.usec;
> + pkts = moder.pkts;
> }
> +
> 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 +1066,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,9 +1413,11 @@ 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;
> +
> INIT_WORK(&dim->dim.work, cb);
> dim->dim.mode = NET_DIM_CQ_PERIOD_MODE_START_FROM_EQE;
> dim->event_ctr = 0;
> @@ -1418,6 +1425,25 @@ static void bcm_sysport_init_dim(struct bcm_sysport_net_dim *dim,
> dim->bytes = 0;
> }
>
> +static void bcm_sysport_init_rx_coalesce(struct bcm_sysport_priv *priv)
> +{
> + struct bcm_sysport_net_dim *dim = &priv->dim;
> + struct net_dim_cq_moder moder;
> + u32 usecs, pkts;
> +
> + usecs = priv->rx_coalesce_usecs;
> + pkts = priv->rx_max_coalesced_frames;
> +
> + /* If DIM was enabled, re-apply default parameters */
> + if (dim->use_dim) {
> + moder = net_dim_get_def_profile(dim->dim.mode);
> + usecs = moder.usec;
> + pkts = moder.pkts;
> + }
> +
> + bcm_sysport_set_rx_coalesce(priv, usecs, pkts);
> +}
> +
> static int bcm_sysport_init_tx_ring(struct bcm_sysport_priv *priv,
> unsigned int index)
> {
> @@ -1658,8 +1684,6 @@ static int bcm_sysport_init_rx_ring(struct bcm_sysport_priv *priv)
> rdma_writel(priv, 0, RDMA_END_ADDR_HI);
> rdma_writel(priv, priv->num_rx_desc_words - 1, RDMA_END_ADDR_LO);
>
> - rdma_writel(priv, 1, RDMA_MBDONE_INTR);
> -
> netif_dbg(priv, hw, priv->netdev,
> "RDMA cfg, num_rx_bds=%d, rx_bds=%p\n",
> priv->num_rx_bds, priv->rx_bds);
> @@ -1827,7 +1851,8 @@ static void bcm_sysport_netif_start(struct net_device *dev)
> struct bcm_sysport_priv *priv = netdev_priv(dev);
>
> /* Enable NAPI */
> - bcm_sysport_init_dim(&priv->dim, bcm_sysport_dim_work);
> + bcm_sysport_init_dim(priv, bcm_sysport_dim_work);
> + bcm_sysport_init_rx_coalesce(priv);
> napi_enable(&priv->napi);
>
> /* Enable RX interrupt and TX ring full interrupt */
> @@ -2333,6 +2358,7 @@ static int bcm_sysport_probe(struct platform_device *pdev)
> /* libphy will adjust the link state accordingly */
> netif_carrier_off(dev);
>
> + priv->rx_max_coalesced_frames = 1;
Do you still want to keep {usecs,frames}={0,1} as default static
configuration? Is the latency so important for the static case? I'm
assuming RT latency is at least 10us so you can somewhat increase the
timer and counter without causing an increase in latency. This should be
properly tested of course so maybe in a future patch.
> u64_stats_init(&priv->syncp);
>
> priv->dsa_notifier.notifier_call = bcm_sysport_dsa_notifier;
> diff --git a/drivers/net/ethernet/broadcom/bcmsysport.h b/drivers/net/ethernet/broadcom/bcmsysport.h
> index 57e18ef8f206..d6e5d0cbf3a3 100644
> --- a/drivers/net/ethernet/broadcom/bcmsysport.h
> +++ b/drivers/net/ethernet/broadcom/bcmsysport.h
> @@ -701,8 +701,6 @@ struct bcm_sysport_net_dim {
> u16 event_ctr;
> unsigned long packets;
> unsigned long bytes;
> - u32 coal_usecs;
> - u32 coal_pkts;
> struct net_dim dim;
> };
>
> @@ -755,6 +753,8 @@ struct bcm_sysport_priv {
> unsigned int rx_c_index;
>
> struct bcm_sysport_net_dim dim;
> + u32 rx_max_coalesced_frames;
> + u32 rx_coalesce_usecs;
>
> /* PHY device */
> struct device_node *phy_dn;
>
Reviewed-by: Tal Gilboa <talgi@...lanox.com>
Powered by blists - more mailing lists