| 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
| ||
|
Message-ID: <pj41zlpm9k27h0.fsf@u570694869fb251.ant.amazon.com>
Date: Mon, 6 Mar 2023 11:27:56 +0200
From: Shay Agroskin <shayagr@...zon.com>
To: Simon Horman <simon.horman@...igine.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
Simon Horman <simon.horman@...igine.com> writes:
> 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>
>
Thanks for reviewing this patchset (:
>> ---
>> 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.
>
Seeing that it is indeed called only once it does make more sense
just to move the function implementation itself
>> 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)
>
Yup, I'll change to it
> ...
>
>> @@ -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.
>
I'll remove it
>> {
>> 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.
>
> ...
>
I'll remove it
>> @@ -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.
>
These are cosmetic little changes to improve code
readability. I'll just create an additional simple commit that
adds them.
>> 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.
>
I'll move it to another patch
>> if (shutdown) {
>> netif_device_detach(netdev);
>> dev_close(netdev);
>
> ...
Powered by blists - more mailing lists