Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1424391 > unrolled thread

Re: [PATCH net-next] net: ena: Add a driver for Amazon Elastic Network Adapters (ENA)

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH net-next] net: ena: Add a driver for Amazon Elastic  Network Adapters (ENA) Francois Romieu <romieu@fr.zoreil.com> - 2016-06-16 22:50 +0200
    Re: [PATCH net-next] net: ena: Add a driver for Amazon Elastic  Network Adapters (ENA) Matt Wilson <msw@amzn.com> - 2016-06-17 03:10 +0200
      Re: [PATCH net-next] net: ena: Add a driver for Amazon Elastic  Network Adapters (ENA) Netanel Belgazal <netanel@annapurnalabs.com> - 2016-06-20 17:20 +0200

#1424391 — Re: [PATCH net-next] net: ena: Add a driver for Amazon Elastic Network Adapters (ENA)

Netanel Belgazal <netanel@annapurnalabs.com> :
[...]

Very limited review below.

> diff --git a/Documentation/networking/ena.txt b/Documentation/networking/ena.txt
> new file mode 100644
> index 0000000..528544f
> --- /dev/null
> +++ b/Documentation/networking/ena.txt
> @@ -0,0 +1,330 @@
> +Linux kernel driver for Elastic Network Adapter (ENA) family:
> +=============================================================
> +
> +Overview:
> +=========
> +The ENA driver provides a modern Ethernet device interface optimized
> +for high performance and low CPU overhead.

Marketing boilerplate. How much of it will still be true in 6 months ?
6 years ?

Hint: stay technical. If in doubt, make it boring. :o)

[...]
> +ENA devices allow high speed and low overhead Ethernet traffic
> +processing by providing a dedicated Tx/Rx queue pair per host CPU, a
> +dedicated MSI-X interrupt vector per Tx/Rx queue pair, adaptive
> +interrupt moderation, and CPU cacheline optimized data placement.

How many queues exactly ?

[...]
> +Memory Allocations:
> +===================
> +DMA Coherent buffers are allocated for the following DMA rings:
> +- Tx submission ring (For regular mode; for LLQ mode it is allocated
> +  using kzalloc)
> +- Tx completion ring
> +- Rx submission ring
> +- Rx completion ring
> +- Admin submission ring
> +- Admin completion ring
> +- AENQ ring

Useless documentation (implementation detail).

Nobody will maintain it when the driver changes. Please remove.

[...]
> +MTU:
> +====
> +The driver supports an arbitrarily large MTU with a maximum that is
> +negotiated with the device. The driver configures MTU using the
> +SetFeature command (ENA_ADMIN_MTU property). The user can change MTU
> +via ifconfig(8) and ip(8).

Please avoid advertising legacy ifconfig.

[...]
> diff --git a/drivers/net/ethernet/amazon/ena/ena_admin_defs.h b/drivers/net/ethernet/amazon/ena/ena_admin_defs.h
> new file mode 100644
> index 0000000..8516e5e
> --- /dev/null
> +++ b/drivers/net/ethernet/amazon/ena/ena_admin_defs.h
[...]
> +/* admin commands opcodes */
> +enum ena_admin_aq_opcode {
> +	/* create submission queue */
> +	ENA_ADMIN_CREATE_SQ = 1,
> +
> +	/* destroy submission queue */
> +	ENA_ADMIN_DESTROY_SQ = 2,
> +
> +	/* create completion queue */
> +	ENA_ADMIN_CREATE_CQ = 3,
> +
> +	/* destroy completion queue */
> +	ENA_ADMIN_DESTROY_CQ = 4,
> +
> +	/* get capabilities of particular feature */
> +	ENA_ADMIN_GET_FEATURE = 8,
> +
> +	/* get capabilities of particular feature */
> +	ENA_ADMIN_SET_FEATURE = 9,
> +
> +	/* get statistics */
> +	ENA_ADMIN_GET_STATS = 11,
> +};

The documentation above simply duplicates the member names -> useless
visual payload.

Please replace space with tab before "=" to line things up.

[...]
> +/* Basic Statistics Command. */
> +struct ena_admin_basic_stats {
> +	/* word 0 :  */
> +	u32 tx_bytes_low;
> +
> +	/* word 1 :  */
> +	u32 tx_bytes_high;
> +
> +	/* word 2 :  */
> +	u32 tx_pkts_low;
> +
> +	/* word 3 :  */
> +	u32 tx_pkts_high;
> +
> +	/* word 4 :  */
> +	u32 rx_bytes_low;
> +
> +	/* word 5 :  */
> +	u32 rx_bytes_high;
> +
> +	/* word 6 :  */
> +	u32 rx_pkts_low;
> +
> +	/* word 7 :  */
> +	u32 rx_pkts_high;
> +
> +	/* word 8 :  */
> +	u32 rx_drops_low;
> +
> +	/* word 9 :  */
> +	u32 rx_drops_high;
> +};

struct zorg {
	u32 ...;
	u32 ...;
	u32 ...;
	u32 ...;

	u32 ...;
	u32 ...;
	u32 ...;
	u32 ...;

	u32 ...;
	u32 ...;
	u32 ...;
};

-> No comment needed to figure the offset.
   Comment removed => no need to check if offset starts at word 0 or word 1.

[...]
> diff --git a/drivers/net/ethernet/amazon/ena/ena_com.c b/drivers/net/ethernet/amazon/ena/ena_com.c
> new file mode 100644
> index 0000000..357e10f
> --- /dev/null
> +++ b/drivers/net/ethernet/amazon/ena/ena_com.c
[...]
> +static inline int ena_com_mem_addr_set(struct ena_com_dev *ena_dev,
> +				       struct ena_common_mem_addr *ena_addr,
> +				       dma_addr_t addr)
> +{
> +	if ((addr & GENMASK_ULL(ena_dev->dma_addr_bits - 1, 0)) != addr) {
> +		ena_trc_err("dma address has more bits that the device supports\n");
> +		return -EINVAL;
> +	}
> +
> +	ena_addr->mem_addr_low = (u32)addr;
> +	ena_addr->mem_addr_high =
> +		((addr & GENMASK_ULL(ena_dev->dma_addr_bits - 1, 32)) >> 32);

No need to "... & GENMASK" given the test above.

> +
> +	return 0;
> +}
> +
> +static int ena_com_admin_init_sq(struct ena_com_admin_queue *queue)
> +{
> +	queue->sq.entries =
> +		dma_alloc_coherent(queue->q_dmadev,
> +				   ADMIN_SQ_SIZE(queue->q_depth),
> +				   &queue->sq.dma_addr,
> +				   GFP_KERNEL | __GFP_ZERO);

1. Use dma_zalloc_coherent().

2. Style
	xyzab = dma_alloc_coherent(..., ..., ...                  ...,
				   ...);

3. Add local variable for &queue->sq.


In a different part of the code: s/uint32_t/u32/

-- 
Ueimor

[toc] | [next] | [standalone]

#1424581

On Thu, Jun 16, 2016 at 10:46:23PM +0200, Francois Romieu wrote:
> Netanel Belgazal <netanel@annapurnalabs.com> :
> [...]
> 
> Very limited review below.

I'll comment on the documentation (since I edited it heavily) but will
leave some the other parts for Netanel to answer.

> > diff --git a/Documentation/networking/ena.txt b/Documentation/networking/ena.txt
> > new file mode 100644
> > index 0000000..528544f
> > --- /dev/null
> > +++ b/Documentation/networking/ena.txt
> > @@ -0,0 +1,330 @@
> > +Linux kernel driver for Elastic Network Adapter (ENA) family:
> > +=============================================================
> > +
> > +Overview:
> > +=========
> > +The ENA driver provides a modern Ethernet device interface optimized
> > +for high performance and low CPU overhead.
> 
> Marketing boilerplate. How much of it will still be true in 6 months ?
> 6 years ?
> 
> Hint: stay technical. If in doubt, make it boring. :o)

Hopefully it will still be true for quite a while. But you make a good
point, the overview here should stick with the technical details.

> [...]
> > +ENA devices allow high speed and low overhead Ethernet traffic
> > +processing by providing a dedicated Tx/Rx queue pair per host CPU, a
> > +dedicated MSI-X interrupt vector per Tx/Rx queue pair, adaptive
> > +interrupt moderation, and CPU cacheline optimized data placement.
> 
> How many queues exactly ?

It's negotiated with the adapter via the admin queue, and it will vary
based on the capabilities of a particular adapter. See:

        rc = ena_com_get_feature(ena_dev, &get_resp,
                                 ENA_ADMIN_MAX_QUEUES_NUM);

in ena_com.c

> [...]
> > +Memory Allocations:
> > +===================
> > +DMA Coherent buffers are allocated for the f`ollowing DMA rings:
> > +- Tx submission ring (For regular mode; for LLQ mode it is allocated
> > +  using kzalloc)
> > +- Tx completion ring
> > +- Rx submission ring
> > +- Rx completion ring
> > +- Admin submission ring
> > +- Admin completion ring
> > +- AENQ ring
> 
> Useless documentation (implementation detail).
> 
> Nobody will maintain it when the driver changes. Please remove.

Ack.

> [...]
> > +MTU:
> > +====
> > +The driver supports an arbitrarily large MTU with a maximum that is
> > +negotiated with the device. The driver configures MTU using the
> > +SetFeature command (ENA_ADMIN_MTU property). The user can change MTU
> > +via ifconfig(8) and ip(8).
> 
> Please avoid advertising legacy ifconfig.

Ack.

> [...]
> > diff --git a/drivers/net/ethernet/amazon/ena/ena_admin_defs.h b/drivers/net/ethernet/amazon/ena/ena_admin_defs.h
> > new file mode 100644
> > index 0000000..8516e5e
> > --- /dev/null
> > +++ b/drivers/net/ethernet/amazon/ena/ena_admin_defs.h
> [...]
> > +/* admin commands opcodes */
> > +enum ena_admin_aq_opcode {
> > +	/* create submission queue */
> > +	ENA_ADMIN_CREATE_SQ = 1,
> > +
> > +	/* destroy submission queue */
> > +	ENA_ADMIN_DESTROY_SQ = 2,
> > +
> > +	/* create completion queue */
> > +	ENA_ADMIN_CREATE_CQ = 3,
> > +
> > +	/* destroy completion queue */
> > +	ENA_ADMIN_DESTROY_CQ = 4,
> > +
> > +	/* get capabilities of particular feature */
> > +	ENA_ADMIN_GET_FEATURE = 8,
> > +
> > +	/* get capabilities of particular feature */
> > +	ENA_ADMIN_SET_FEATURE = 9,
> > +
> > +	/* get statistics */
> > +	ENA_ADMIN_GET_STATS = 11,
> > +};
> 
> The documentation above simply duplicates the member names -> useless
> visual payload.
> 
> Please replace space with tab before "=" to line things up.

I'll leave this to Netanel. A good amount of this comes from a unified
documentation / struct / register access generation script, and it
might need some massaging.

> [...]
> > +/* Basic Statistics Command. */
> > +struct ena_admin_basic_stats {
> > +	/* word 0 :  */
> > +	u32 tx_bytes_low;
> > +
> > +	/* word 1 :  */
> > +	u32 tx_bytes_high;
> > +
> > +	/* word 2 :  */
> > +	u32 tx_pkts_low;
> > +
> > +	/* word 3 :  */
> > +	u32 tx_pkts_high;
> > +
> > +	/* word 4 :  */
> > +	u32 rx_bytes_low;
> > +
> > +	/* word 5 :  */
> > +	u32 rx_bytes_high;
> > +
> > +	/* word 6 :  */
> > +	u32 rx_pkts_low;
> > +
> > +	/* word 7 :  */
> > +	u32 rx_pkts_high;
> > +
> > +	/* word 8 :  */
> > +	u32 rx_drops_low;
> > +
> > +	/* word 9 :  */
> > +	u32 rx_drops_high;
> > +};
> 
> struct zorg {
> 	u32 ...;
> 	u32 ...;
> 	u32 ...;
> 	u32 ...;
> 
> 	u32 ...;
> 	u32 ...;
> 	u32 ...;
> 	u32 ...;
> 
> 	u32 ...;
> 	u32 ...;
> 	u32 ...;
> };
> 
> -> No comment needed to figure the offset.
>    Comment removed => no need to check if offset starts at word 0 or word 1.
> 
> [...]
> > diff --git a/drivers/net/ethernet/amazon/ena/ena_com.c b/drivers/net/ethernet/amazon/ena/ena_com.c
> > new file mode 100644
> > index 0000000..357e10f
> > --- /dev/null
> > +++ b/drivers/net/ethernet/amazon/ena/ena_com.c
> [...]
> > +static inline int ena_com_mem_addr_set(struct ena_com_dev *ena_dev,
> > +				       struct ena_common_mem_addr *ena_addr,
> > +				       dma_addr_t addr)
> > +{
> > +	if ((addr & GENMASK_ULL(ena_dev->dma_addr_bits - 1, 0)) != addr) {
> > +		ena_trc_err("dma address has more bits that the device supports\n");
> > +		return -EINVAL;
> > +	}
> > +
> > +	ena_addr->mem_addr_low = (u32)addr;
> > +	ena_addr->mem_addr_high =
> > +		((addr & GENMASK_ULL(ena_dev->dma_addr_bits - 1, 32)) >> 32);
> 
> No need to "... & GENMASK" given the test above.

Ack.

> > +
> > +	return 0;
> > +}
> > +
> > +static int ena_com_admin_init_sq(struct ena_com_admin_queue *queue)
> > +{
> > +	queue->sq.entries =
> > +		dma_alloc_coherent(queue->q_dmadev,
> > +				   ADMIN_SQ_SIZE(queue->q_depth),
> > +				   &queue->sq.dma_addr,
> > +				   GFP_KERNEL | __GFP_ZERO);
> 
> 1. Use dma_zalloc_coherent().
> 
> 2. Style
> 	xyzab = dma_alloc_coherent(..., ..., ...                  ...,
> 				   ...);
> 
> 3. Add local variable for &queue->sq.

Ack.

> In a different part of the code: s/uint32_t/u32/

Argh! I thought I caught all of those.

Thanks for the review!

--msw

[toc] | [prev] | [next] | [standalone]

#1426684

On 06/17/2016 04:09 AM, Matt Wilson wrote:
> On Thu, Jun 16, 2016 at 10:46:23PM +0200, Francois Romieu wrote:
>> Netanel Belgazal <netanel@annapurnalabs.com> :
>> [...]
>>
>> Very limited review below.
> I'll comment on the documentation (since I edited it heavily) but will
> leave some the other parts for Netanel to answer.
>
>>> diff --git a/Documentation/networking/ena.txt b/Documentation/networking/ena.txt
>>> new file mode 100644
>>> index 0000000..528544f
>>> --- /dev/null
>>> +++ b/Documentation/networking/ena.txt
>>> @@ -0,0 +1,330 @@
>>> +Linux kernel driver for Elastic Network Adapter (ENA) family:
>>> +=============================================================
>>> +
>>> +Overview:
>>> +=========
>>> +The ENA driver provides a modern Ethernet device interface optimized
>>> +for high performance and low CPU overhead.
>> Marketing boilerplate. How much of it will still be true in 6 months ?
>> 6 years ?
>>
>> Hint: stay technical. If in doubt, make it boring. :o)
> Hopefully it will still be true for quite a while. But you make a good
> point, the overview here should stick with the technical details.
>
>> [...]
>>> +ENA devices allow high speed and low overhead Ethernet traffic
>>> +processing by providing a dedicated Tx/Rx queue pair per host CPU, a
>>> +dedicated MSI-X interrupt vector per Tx/Rx queue pair, adaptive
>>> +interrupt moderation, and CPU cacheline optimized data placement.
>> How many queues exactly ?
> It's negotiated with the adapter via the admin queue, and it will vary
> based on the capabilities of a particular adapter. See:
>
>         rc = ena_com_get_feature(ena_dev, &get_resp,
>                                  ENA_ADMIN_MAX_QUEUES_NUM);
>
> in ena_com.c
>
>> [...]
>>> +Memory Allocations:
>>> +===================
>>> +DMA Coherent buffers are allocated for the f`ollowing DMA rings:
>>> +- Tx submission ring (For regular mode; for LLQ mode it is allocated
>>> +  using kzalloc)
>>> +- Tx completion ring
>>> +- Rx submission ring
>>> +- Rx completion ring
>>> +- Admin submission ring
>>> +- Admin completion ring
>>> +- AENQ ring
>> Useless documentation (implementation detail).
>>
>> Nobody will maintain it when the driver changes. Please remove.
> Ack.
>
>> [...]
>>> +MTU:
>>> +====
>>> +The driver supports an arbitrarily large MTU with a maximum that is
>>> +negotiated with the device. The driver configures MTU using the
>>> +SetFeature command (ENA_ADMIN_MTU property). The user can change MTU
>>> +via ifconfig(8) and ip(8).
>> Please avoid advertising legacy ifconfig.
> Ack.
>
>> [...]
>>> diff --git a/drivers/net/ethernet/amazon/ena/ena_admin_defs.h b/drivers/net/ethernet/amazon/ena/ena_admin_defs.h
>>> new file mode 100644
>>> index 0000000..8516e5e
>>> --- /dev/null
>>> +++ b/drivers/net/ethernet/amazon/ena/ena_admin_defs.h
>> [...]
>>> +/* admin commands opcodes */
>>> +enum ena_admin_aq_opcode {
>>> +	/* create submission queue */
>>> +	ENA_ADMIN_CREATE_SQ = 1,
>>> +
>>> +	/* destroy submission queue */
>>> +	ENA_ADMIN_DESTROY_SQ = 2,
>>> +
>>> +	/* create completion queue */
>>> +	ENA_ADMIN_CREATE_CQ = 3,
>>> +
>>> +	/* destroy completion queue */
>>> +	ENA_ADMIN_DESTROY_CQ = 4,
>>> +
>>> +	/* get capabilities of particular feature */
>>> +	ENA_ADMIN_GET_FEATURE = 8,
>>> +
>>> +	/* get capabilities of particular feature */
>>> +	ENA_ADMIN_SET_FEATURE = 9,
>>> +
>>> +	/* get statistics */
>>> +	ENA_ADMIN_GET_STATS = 11,
>>> +};
>> The documentation above simply duplicates the member names -> useless
>> visual payload.
>>
>> Please replace space with tab before "=" to line things up
> I'll leave this to Netanel. A good amount of this comes from a unified
> documentation / struct / register access generation script, and it
> might need some massaging.
Ack. (Removed comments and replace space with tab)
>
>> [...]
>>> +/* Basic Statistics Command. */
>>> +struct ena_admin_basic_stats {
>>> +	/* word 0 :  */
>>> +	u32 tx_bytes_low;
>>> +
>>> +	/* word 1 :  */
>>> +	u32 tx_bytes_high;
>>> +
>>> +	/* word 2 :  */
>>> +	u32 tx_pkts_low;
>>> +
>>> +	/* word 3 :  */
>>> +	u32 tx_pkts_high;
>>> +
>>> +	/* word 4 :  */
>>> +	u32 rx_bytes_low;
>>> +
>>> +	/* word 5 :  */
>>> +	u32 rx_bytes_high;
>>> +
>>> +	/* word 6 :  */
>>> +	u32 rx_pkts_low;
>>> +
>>> +	/* word 7 :  */
>>> +	u32 rx_pkts_high;
>>> +
>>> +	/* word 8 :  */
>>> +	u32 rx_drops_low;
>>> +
>>> +	/* word 9 :  */
>>> +	u32 rx_drops_high;
>>> +};
>> struct zorg {
>> 	u32 ...;
>> 	u32 ...;
>> 	u32 ...;
>> 	u32 ...;
>>
>> 	u32 ...;
>> 	u32 ...;
>> 	u32 ...;
>> 	u32 ...;
>>
>> 	u32 ...;
>> 	u32 ...;
>> 	u32 ...;
>> };
>>
>> -> No comment needed to figure the offset.
>>    Comment removed => no need to check if offset starts at word 0 or word 1.
>>
>> [...]
Ack
>>> diff --git a/drivers/net/ethernet/amazon/ena/ena_com.c b/drivers/net/ethernet/amazon/ena/ena_com.c
>>> new file mode 100644
>>> index 0000000..357e10f
>>> --- /dev/null
>>> +++ b/drivers/net/ethernet/amazon/ena/ena_com.c
>> [...]
>>> +static inline int ena_com_mem_addr_set(struct ena_com_dev *ena_dev,
>>> +				       struct ena_common_mem_addr *ena_addr,
>>> +				       dma_addr_t addr)
>>> +{
>>> +	if ((addr & GENMASK_ULL(ena_dev->dma_addr_bits - 1, 0)) != addr) {
>>> +		ena_trc_err("dma address has more bits that the device supports\n");
>>> +		return -EINVAL;
>>> +	}
>>> +
>>> +	ena_addr->mem_addr_low = (u32)addr;
>>> +	ena_addr->mem_addr_high =
>>> +		((addr & GENMASK_ULL(ena_dev->dma_addr_bits - 1, 32)) >> 32);
>> No need to "... & GENMASK" given the test above.
> Ack.
>
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static int ena_com_admin_init_sq(struct ena_com_admin_queue *queue)
>>> +{
>>> +	queue->sq.entries =
>>> +		dma_alloc_coherent(queue->q_dmadev,
>>> +				   ADMIN_SQ_SIZE(queue->q_depth),
>>> +				   &queue->sq.dma_addr,
>>> +				   GFP_KERNEL | __GFP_ZERO);
>> 1. Use dma_zalloc_coherent().
>>
>> 2. Style
>> 	xyzab = dma_alloc_coherent(..., ..., ...                  ...,
>> 				   ...);
>>
>> 3. Add local variable for &queue->sq.
> Ack.
>
>> In a different part of the code: s/uint32_t/u32/
> Argh! I thought I caught all of those.
Ack
>
> Thanks for the review!
>
> --msw

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web