netdev - Re: [net-next 09/10] bnxt_en: add support for software dynamic interrupt moderation

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 4 Jan 2018 14:16:26 -0800
From:   Michael Chan <michael.chan@...adcom.com>
To:     Andy Gospodarek <andy@...yhouse.net>
Cc:     Netdev <netdev@...r.kernel.org>,
        "michael.chan@...adcom.com" <mchan@...adcom.com>,
        talgi@...lanox.com, ogerlitz@...lanox.com,
        Andy Gospodarek <gospo@...adcom.com>
Subject: Re: [net-next 09/10] bnxt_en: add support for software dynamic
 interrupt moderation

On Thu, Jan 4, 2018 at 12:21 PM, Andy Gospodarek <andy@...yhouse.net> wrote:
> From: Andy Gospodarek <gospo@...adcom.com>
>
> This implements the changes needed for the bnxt_en driver to add support
> for dynamic interrupt moderation per ring.
>
> This does add additional counters in the receive path, but testing shows
> that any additional instructions are offset by throughput gain when the
> default configuration is for low latency.
>
> Signed-off-by: Andy Gospodarek <gospo@...adcom.com>
> Cc: Michael Chan <mchan@...adcom.com>

Andy, looks good in general. I just have a few comments below.  These
minor issues can be cleaned up after merge if you want.

....
> +int bnxt_hwrm_set_ring_coal(struct bnxt *bp, struct bnxt_napi *bnapi)
> +{
> +       struct hwrm_ring_cmpl_ring_cfg_aggint_params_input req_rx = {0};
> +       struct bnxt_cp_ring_info *cpr = &bnapi->cp_ring;
> +       struct bnxt_coal coal;
> +       unsigned int grp_idx;
> +       int rc = 0;
> +
> +        /* Tick values in micro seconds.
> +         * 1 coal_buf x bufs_per_record = 1 completion record.
> +         */
> +       memcpy(&coal, &bp->rx_coal, sizeof(struct bnxt_coal));
> +
> +       coal.coal_ticks = cpr->rx_ring_coal.coal_ticks;
> +       coal.coal_bufs = cpr->rx_ring_coal.coal_bufs;
> +
> +       if (!bnapi->rx_ring)
> +               return -ENODEV;
> +
> +       bnxt_hwrm_cmd_hdr_init(bp, &req_rx,
> +                              HWRM_RING_CMPL_RING_CFG_AGGINT_PARAMS, -1, -1);
> +
> +       bnxt_hwrm_set_coal_params(&coal, &req_rx);
> +
> +       mutex_lock(&bp->hwrm_cmd_lock);
> +       grp_idx = bnapi->index;
> +
> +       req_rx.ring_id = cpu_to_le16(bp->grp_info[grp_idx].cp_fw_ring_id);
> +
> +       rc = _hwrm_send_message(bp, &req_rx, sizeof(req_rx),
> +                               HWRM_CMD_TIMEOUT);
> +       mutex_unlock(&bp->hwrm_cmd_lock);

You can use the hwrm_send_message() variant that does not require you
to take the mutex.  You only need this variant and take the mutex if
you need to check the firmware reply.

> +       return rc;
> +}
> +
>  int bnxt_hwrm_set_coal(struct bnxt *bp)
>  {
>         int i, rc = 0;
> @@ -5705,7 +5753,11 @@ static void bnxt_enable_napi(struct bnxt *bp)
>         int i;
>
>         for (i = 0; i < bp->cp_nr_rings; i++) {

We only need to enable this for every completion ring that has an RX
ring.  In some cases, for example when XDP is enabled, there will be a
set of completion rings with only TX rings.  So I think we can
optimize this for completion rings with RX only.

> +               struct bnxt_cp_ring_info *cpr = &bp->bnapi[i]->cp_ring;
>                 bp->bnapi[i]->in_reset = false;
> +
> +               INIT_WORK(&cpr->am.work, bnxt_dim_work);
> +               cpr->am.mode = NET_DIM_CQ_PERIOD_MODE_START_FROM_EQE;
>                 napi_enable(&bp->bnapi[i]->napi);
>         }
>  }