From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andy Gospodarek Subject: Re: [net-next 09/10] bnxt_en: add support for software dynamic interrupt moderation Date: Fri, 5 Jan 2018 08:44:57 -0500 Message-ID: <20180105134457.GA78313@C02RW35GFVH8> References: <1515097290-17470-1-git-send-email-andy@greyhouse.net> <1515097290-17470-10-git-send-email-andy@greyhouse.net> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: Netdev , "michael.chan@broadcom.com" , talgi@mellanox.com, ogerlitz@mellanox.com, Andy Gospodarek To: Michael Chan Return-path: Received: from mail-qk0-f196.google.com ([209.85.220.196]:33985 "EHLO mail-qk0-f196.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751787AbeAENpB (ORCPT ); Fri, 5 Jan 2018 08:45:01 -0500 Received: by mail-qk0-f196.google.com with SMTP id g81so5954824qke.1 for ; Fri, 05 Jan 2018 05:45:00 -0800 (PST) Content-Disposition: inline In-Reply-To: Sender: netdev-owner@vger.kernel.org List-ID: On Thu, Jan 04, 2018 at 02:16:26PM -0800, Michael Chan wrote: > On Thu, Jan 4, 2018 at 12:21 PM, Andy Gospodarek wrote: > > From: Andy Gospodarek > > > > 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 > > Cc: Michael Chan > > Andy, looks good in general. I just have a few comments below. These > minor issues can be cleaned up after merge if you want. Thanks for the review -- not the first time you've seen it :-) -- and for agreeing that we can cleanup after the merge. I'll need a v2, so I might as well fix anything we want to fix now. > > .... > > +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. > OK, good to know. I'll consider whether or not it is important to check the reply. I think I'd want to know if it failed, but I'm not sure what I'd do were that error condition encountered.... > > + 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. Good call. > > + 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); > > } > > }