Re: [PATCH RFC v2 net-next 2/4] net: ena: Add an option to configure large LLQ headers

[Simon Horman <simon.horman@corigine.com>]

On Thu, Mar 02, 2023 at 10:30:43PM +0200, Shay Agroskin wrote:
> From: David Arinzon <darinzon@amazon.com>
> 
> Allow configuring the device with large LLQ headers. The Low Latency
> Queue (LLQ) allows the driver to write the first N bytes of the packet,
> along with the rest of the TX descriptors directly into device (N can be
> either 96 or 224 for large LLQ headers configuration).
> 
> Having L4 TCP/UDP headers contained in the first 96 bytes of the packet
> is required to get maximum performance from the device.
> 
> Signed-off-by: David Arinzon <darinzon@amazon.com>
> Signed-off-by: Shay Agroskin <shayagr@amazon.com>

Overall this looks very nice to me, its a very interesting HW feature.

As this is an RFC I've made a few nit-picking comments inline.
Those not withstanding,

Reviewed-by: Simon Horman <simon.horman@corigine.com>

> ---
>  drivers/net/ethernet/amazon/ena/ena_netdev.c | 100 ++++++++++++++-----
>  drivers/net/ethernet/amazon/ena/ena_netdev.h |   8 ++
>  2 files changed, 84 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/net/ethernet/amazon/ena/ena_netdev.c b/drivers/net/ethernet/amazon/ena/ena_netdev.c
> index d3999db7c6a2..830d5be22aa9 100644
> --- a/drivers/net/ethernet/amazon/ena/ena_netdev.c
> +++ b/drivers/net/ethernet/amazon/ena/ena_netdev.c
> @@ -44,6 +44,8 @@ static int ena_rss_init_default(struct ena_adapter *adapter);
>  static void check_for_admin_com_state(struct ena_adapter *adapter);
>  static void ena_destroy_device(struct ena_adapter *adapter, bool graceful);
>  static int ena_restore_device(struct ena_adapter *adapter);
> +static void ena_calc_io_queue_size(struct ena_adapter *adapter,
> +				   struct ena_com_dev_get_features_ctx *get_feat_ctx);
>  

FWIIW, I think it is nicer to move functions rather than provide forward
declarations. That could be done in a preparatory patch if you want
to avoid crowding out the intentions of this this patch.

>  static void ena_init_io_rings(struct ena_adapter *adapter,
>  			      int first_index, int count);
> @@ -3387,13 +3389,30 @@ static int ena_device_validate_params(struct ena_adapter *adapter,
>  	return 0;
>  }
>  
> -static void set_default_llq_configurations(struct ena_llq_configurations *llq_config)
> +static void set_default_llq_configurations(struct ena_adapter *adapter,
> +					   struct ena_llq_configurations *llq_config,
> +					   struct ena_admin_feature_llq_desc *llq)
>  {
> +	struct ena_com_dev *ena_dev = adapter->ena_dev;
> +
>  	llq_config->llq_header_location = ENA_ADMIN_INLINE_HEADER;
>  	llq_config->llq_stride_ctrl = ENA_ADMIN_MULTIPLE_DESCS_PER_ENTRY;
>  	llq_config->llq_num_decs_before_header = ENA_ADMIN_LLQ_NUM_DESCS_BEFORE_HEADER_2;
> -	llq_config->llq_ring_entry_size = ENA_ADMIN_LIST_ENTRY_SIZE_128B;
> -	llq_config->llq_ring_entry_size_value = 128;
> +
> +	adapter->large_llq_header_supported =
> +		!!(ena_dev->supported_features & (1 << ENA_ADMIN_LLQ));

nit: BIT(ENA_ADMIN_LLQ)

...

> @@ -3587,7 +3609,8 @@ static int ena_enable_msix_and_set_admin_interrupts(struct ena_adapter *adapter)
>  	return rc;
>  }
>  
> -static void ena_destroy_device(struct ena_adapter *adapter, bool graceful)
> +static
> +void ena_destroy_device(struct ena_adapter *adapter, bool graceful)

nit: this change seems unrelated to the rest of this patch.

>  {
>  	struct net_device *netdev = adapter->netdev;
>  	struct ena_com_dev *ena_dev = adapter->ena_dev;
> @@ -3633,7 +3656,8 @@ static void ena_destroy_device(struct ena_adapter *adapter, bool graceful)
>  	clear_bit(ENA_FLAG_DEVICE_RUNNING, &adapter->flags);
>  }
>  
> -static int ena_restore_device(struct ena_adapter *adapter)
> +static
> +int ena_restore_device(struct ena_adapter *adapter)

Ditto.

...

> @@ -4333,7 +4384,6 @@ static int ena_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>  	ena_dev->intr_moder_rx_interval = ENA_INTR_INITIAL_RX_INTERVAL_USECS;
>  	ena_dev->intr_delay_resolution = ENA_DEFAULT_INTR_DELAY_RESOLUTION;
>  	max_num_io_queues = ena_calc_max_io_queue_num(pdev, ena_dev, &get_feat_ctx);
> -	ena_calc_io_queue_size(adapter, &get_feat_ctx);
>  	if (unlikely(!max_num_io_queues)) {
>  		rc = -EFAULT;
>  		goto err_device_destroy;
> @@ -4366,6 +4416,7 @@ static int ena_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>  			"Failed to query interrupt moderation feature\n");
>  		goto err_device_destroy;
>  	}
> +

nit: this change seems unrelated to the rest of this patch.

>  	ena_init_io_rings(adapter,
>  			  0,
>  			  adapter->xdp_num_queues +
> @@ -4486,6 +4537,7 @@ static void __ena_shutoff(struct pci_dev *pdev, bool shutdown)
>  	rtnl_lock(); /* lock released inside the below if-else block */
>  	adapter->reset_reason = ENA_REGS_RESET_SHUTDOWN;
>  	ena_destroy_device(adapter, true);
> +

Ditto.

>  	if (shutdown) {
>  		netif_device_detach(netdev);
>  		dev_close(netdev);

...