Re: [PATCH V7 6/8] i40e: queue-specific settings for interrupt moderation

[Jeff Kirsher <jeffrey.t.kirsher@intel.com>]

[-- Attachment #1: Type: text/plain, Size: 6815 bytes --]

On Fri, 2016-02-19 at 09:24 -0500, Kan Liang wrote:
> From: Kan Liang <kan.liang@intel.com>
> 
> For i40e driver, each vector has its own ITR register. However, there
> are no concept of queue-specific settings in the driver proper. Only
> global variable is used to store ITR values. That will cause problems
> especially when resetting the vector. The specific ITR values could
> be
> lost.
> This patch move rx_itr_setting and tx_itr_setting to i40e_ring to
> store
> specific ITR register for each queue.
> i40e_get_coalesce and i40e_set_coalesce are also modified accordingly
> to
> support queue-specific settings. To make it compatible with old
> ethtool,
> if user doesn't specify the queue number, i40e_get_coalesce will
> return
> queue 0's value. While i40e_set_coalesce will apply value to all
> queues.
> 
> Signed-off-by: Kan Liang <kan.liang@intel.com>
> Acked-by: Shannon Nelson <shannon.nelson@intel.com>

Acked-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>

There is one minor nitpick noted below, but that should not hold up
this patch.

> ---
>  drivers/net/ethernet/intel/i40e/i40e.h         |   7 --
>  drivers/net/ethernet/intel/i40e/i40e_debugfs.c |  15 ++-
>  drivers/net/ethernet/intel/i40e/i40e_ethtool.c | 139
> ++++++++++++++++---------
>  drivers/net/ethernet/intel/i40e/i40e_main.c    |  12 +--
>  drivers/net/ethernet/intel/i40e/i40e_txrx.c    |   9 +-
>  drivers/net/ethernet/intel/i40e/i40e_txrx.h    |   8 ++
>  6 files changed, 120 insertions(+), 70 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/i40e/i40e.h
> b/drivers/net/ethernet/intel/i40e/i40e.h
> index e99be9f..2f6210a 100644
> --- a/drivers/net/ethernet/intel/i40e/i40e.h
> +++ b/drivers/net/ethernet/intel/i40e/i40e.h
> @@ -521,13 +521,6 @@ struct i40e_vsi {
>         struct i40e_ring **tx_rings;
>  
>         u16 work_limit;
> -       /* high bit set means dynamic, use accessor routines to
> read/write.
> -        * hardware only supports 2us resolution for the ITR
> registers.
> -        * these values always store the USER setting, and must be
> converted
> -        * before programming to a register.
> -        */
> -       u16 rx_itr_setting;
> -       u16 tx_itr_setting;
>         u16 int_rate_limit;  /* value in usecs */
>  
>         u16 rss_table_size; /* HW RSS table size */
> diff --git a/drivers/net/ethernet/intel/i40e/i40e_debugfs.c
> b/drivers/net/ethernet/intel/i40e/i40e_debugfs.c
> index 2a44f2e..0c97733 100644
> --- a/drivers/net/ethernet/intel/i40e/i40e_debugfs.c
> +++ b/drivers/net/ethernet/intel/i40e/i40e_debugfs.c
> @@ -302,6 +302,10 @@ static void i40e_dbg_dump_vsi_seid(struct
> i40e_pf *pf, int seid)
>                          "    rx_rings[%i]: vsi = %p, q_vector =
> %p\n",
>                          i, rx_ring->vsi,
>                          rx_ring->q_vector);
> +               dev_info(&pf->pdev->dev,
> +                        "    rx_rings[%i]: rx_itr_setting = %d
> (%s)\n",
> +                        i, rx_ring->rx_itr_setting,
> +                        ITR_IS_DYNAMIC(rx_ring->rx_itr_setting) ?
> "dynamic" : "fixed");
>         }
>         for (i = 0; i < vsi->num_queue_pairs; i++) {
>                 struct i40e_ring *tx_ring = ACCESS_ONCE(vsi-
> >tx_rings[i]);
> @@ -352,14 +356,15 @@ static void i40e_dbg_dump_vsi_seid(struct
> i40e_pf *pf, int seid)
>                 dev_info(&pf->pdev->dev,
>                          "    tx_rings[%i]: DCB tc = %d\n",
>                          i, tx_ring->dcb_tc);
> +               dev_info(&pf->pdev->dev,
> +                        "    tx_rings[%i]: tx_itr_setting = %d
> (%s)\n",
> +                        i, tx_ring->tx_itr_setting,
> +                        ITR_IS_DYNAMIC(tx_ring->tx_itr_setting) ?
> "dynamic" : "fixed");
>         }
>         rcu_read_unlock();
>         dev_info(&pf->pdev->dev,
> -                "    work_limit = %d, rx_itr_setting = %d (%s),
> tx_itr_setting = %d (%s)\n",
> -                vsi->work_limit, vsi->rx_itr_setting,
> -                ITR_IS_DYNAMIC(vsi->rx_itr_setting) ? "dynamic" :
> "fixed",
> -                vsi->tx_itr_setting,
> -                ITR_IS_DYNAMIC(vsi->tx_itr_setting) ? "dynamic" :
> "fixed");
> +                "    work_limit = %d\n",
> +                vsi->work_limit);
>         dev_info(&pf->pdev->dev,
>                  "    max_frame = %d, rx_hdr_len = %d, rx_buf_len =
> %d dtype = %d\n",
>                  vsi->max_frame, vsi->rx_hdr_len, vsi->rx_buf_len,
> vsi->dtype);
> diff --git a/drivers/net/ethernet/intel/i40e/i40e_ethtool.c
> b/drivers/net/ethernet/intel/i40e/i40e_ethtool.c
> index a85bc94..a470599 100644
> --- a/drivers/net/ethernet/intel/i40e/i40e_ethtool.c
> +++ b/drivers/net/ethernet/intel/i40e/i40e_ethtool.c
> @@ -1879,8 +1879,9 @@ static int i40e_set_phys_id(struct net_device
> *netdev,
>   * 125us (8000 interrupts per second) == ITR(62)
>   */
>  
> -static int i40e_get_coalesce(struct net_device *netdev,
> -                            struct ethtool_coalesce *ec)
> +static int __i40e_get_coalesce(struct net_device *netdev,
> +                              struct ethtool_coalesce *ec,
> +                              int queue)
>  {
>         struct i40e_netdev_priv *np = netdev_priv(netdev);
>         struct i40e_vsi *vsi = np->vsi;
> @@ -1888,14 +1889,24 @@ static int i40e_get_coalesce(struct
> net_device *netdev,
>         ec->tx_max_coalesced_frames_irq = vsi->work_limit;
>         ec->rx_max_coalesced_frames_irq = vsi->work_limit;
>  
> -       if (ITR_IS_DYNAMIC(vsi->rx_itr_setting))
> +       /* rx and tx usecs has per queue value. If user doesn't
> specify the queue,
> +        * return queue 0's value to represent.
> +        */
> +       if (queue < 0) {
> +               queue = 0;
> +       } else if (queue >= vsi->num_queue_pairs) {
> +               return -EINVAL;
> +       }
> +
> +       if (ITR_IS_DYNAMIC(vsi->rx_rings[queue]->rx_itr_setting))

The curly braces are not needed in the above if/else if statement.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]