| 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
| ||
|
Date: Fri, 3 Mar 2023 12:51:48 +0100
From: Simon Horman <simon.horman@...igine.com>
To: Shay Agroskin <shayagr@...zon.com>
Cc: David Miller <davem@...emloft.net>,
Jakub Kicinski <kuba@...nel.org>, netdev@...r.kernel.org,
David Arinzon <darinzon@...zon.com>,
"Woodhouse, David" <dwmw@...zon.com>,
"Machulsky, Zorik" <zorik@...zon.com>,
"Matushevsky, Alexander" <matua@...zon.com>,
Saeed Bshara <saeedb@...zon.com>,
"Wilson, Matt" <msw@...zon.com>,
"Liguori, Anthony" <aliguori@...zon.com>,
"Bshara, Nafea" <nafea@...zon.com>,
"Belgazal, Netanel" <netanel@...zon.com>,
"Saidi, Ali" <alisaidi@...zon.com>,
"Herrenschmidt, Benjamin" <benh@...zon.com>,
"Kiyanovski, Arthur" <akiyano@...zon.com>,
"Dagan, Noam" <ndagan@...zon.com>,
"Itzko, Shahar" <itzko@...zon.com>,
"Abboud, Osama" <osamaabb@...zon.com>
Subject: Re: [PATCH RFC v2 net-next 2/4] net: ena: Add an option to configure
large LLQ headers
On Thu, Mar 02, 2023 at 10:30:43PM +0200, Shay Agroskin wrote:
> From: David Arinzon <darinzon@...zon.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@...zon.com>
> Signed-off-by: Shay Agroskin <shayagr@...zon.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@...igine.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);
...
Powered by blists - more mailing lists