[V3,net-next,1/9] net: ena: use constant value for net_device allocation

Commit Message

Kiyanovski, Arthur Dec. 2, 2020, 8:03 p.m. UTC

From: Arthur Kiyanovski <[e-mail]>

The patch changes the maximum number of RX/TX queues it advertises to
the kernel (via alloc_etherdev_mq()) from a value received from the
device to a constant value which is the minimum between 128 and the
number of CPUs in the system.

By allocating the net_device struct with a constant number of queues,
the driver is able to allocate it at a much earlier stage, before
calling any ena_com functions. This would allow to make all log prints in
ena_com to use netdev_* log functions instead or current pr_* ones.

Signed-off-by: Shay Agroskin <[e-mail]>
Signed-off-by: Arthur Kiyanovski <[e-mail]>
---
 drivers/net/ethernet/amazon/ena/ena_netdev.c | 46 ++++++++++----------
 1 file changed, 23 insertions(+), 23 deletions(-)

Comments

Heiner Kallweit Dec. 2, 2020, 9:55 p.m. UTC | #1

Am 02.12.2020 um 21:03 schrieb [e-mail]:
> From: Arthur Kiyanovski <[e-mail]>
> 
> The patch changes the maximum number of RX/TX queues it advertises to
> the kernel (via alloc_etherdev_mq()) from a value received from the
> device to a constant value which is the minimum between 128 and the
> number of CPUs in the system.
> 
> By allocating the net_device struct with a constant number of queues,
> the driver is able to allocate it at a much earlier stage, before
> calling any ena_com functions. This would allow to make all log prints in
> ena_com to use netdev_* log functions instead or current pr_* ones.
> 

Did you test this? Usually using netdev_* before the net_device is
registered results in quite ugly messages. Therefore there's a number
of patches doing the opposite, replacing netdev_* with dev_* before
register_netdev(). See e.g.
22148df0d0bd ("r8169: don't use netif_info et al before net_device has been registered")


> Signed-off-by: Shay Agroskin <[e-mail]>
> Signed-off-by: Arthur Kiyanovski <[e-mail]>
> ---
>  drivers/net/ethernet/amazon/ena/ena_netdev.c | 46 ++++++++++----------
>  1 file changed, 23 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/net/ethernet/amazon/ena/ena_netdev.c b/drivers/net/ethernet/amazon/ena/ena_netdev.c
> index df1884d57d1a..985dea1870b5 100644
> --- a/drivers/net/ethernet/amazon/ena/ena_netdev.c
> +++ b/drivers/net/ethernet/amazon/ena/ena_netdev.c
> @@ -29,6 +29,8 @@ MODULE_LICENSE("GPL");
>  /* Time in jiffies before concluding the transmitter is hung. */
>  #define TX_TIMEOUT  (5 * HZ)
>  
> +#define ENA_MAX_RINGS min_t(unsigned int, ENA_MAX_NUM_IO_QUEUES, num_possible_cpus())
> +
>  #define ENA_NAPI_BUDGET 64
>  
>  #define DEFAULT_MSG_ENABLE (NETIF_MSG_DRV | NETIF_MSG_PROBE | NETIF_MSG_IFUP | \
> @@ -4176,18 +4178,34 @@ static int ena_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>  
>  	ena_dev->dmadev = &pdev->dev;
>  
> +	netdev = alloc_etherdev_mq(sizeof(struct ena_adapter), ENA_MAX_RINGS);
> +	if (!netdev) {
> +		dev_err(&pdev->dev, "alloc_etherdev_mq failed\n");
> +		rc = -ENOMEM;
> +		goto err_free_region;
> +	}
> +
> +	SET_NETDEV_DEV(netdev, &pdev->dev);
> +	adapter = netdev_priv(netdev);
> +	adapter->ena_dev = ena_dev;
> +	adapter->netdev = netdev;
> +	adapter->pdev = pdev;
> +	adapter->msg_enable = netif_msg_init(debug, DEFAULT_MSG_ENABLE);
> +
> +	pci_set_drvdata(pdev, adapter);
> +
>  	rc = ena_device_init(ena_dev, pdev, &get_feat_ctx, &wd_state);
>  	if (rc) {
>  		dev_err(&pdev->dev, "ENA device init failed\n");
>  		if (rc == -ETIME)
>  			rc = -EPROBE_DEFER;
> -		goto err_free_region;
> +		goto err_netdev_destroy;
>  	}
>  
>  	rc = ena_map_llq_mem_bar(pdev, ena_dev, bars);
>  	if (rc) {
>  		dev_err(&pdev->dev, "ENA llq bar mapping failed\n");
> -		goto err_free_ena_dev;
> +		goto err_device_destroy;
>  	}
>  
>  	calc_queue_ctx.ena_dev = ena_dev;
> @@ -4207,26 +4225,8 @@ static int ena_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>  		goto err_device_destroy;
>  	}
>  
> -	/* dev zeroed in init_etherdev */
> -	netdev = alloc_etherdev_mq(sizeof(struct ena_adapter), max_num_io_queues);
> -	if (!netdev) {
> -		dev_err(&pdev->dev, "alloc_etherdev_mq failed\n");
> -		rc = -ENOMEM;
> -		goto err_device_destroy;
> -	}
> -
> -	SET_NETDEV_DEV(netdev, &pdev->dev);
> -
> -	adapter = netdev_priv(netdev);
> -	pci_set_drvdata(pdev, adapter);
> -
> -	adapter->ena_dev = ena_dev;
> -	adapter->netdev = netdev;
> -	adapter->pdev = pdev;
> -
>  	ena_set_conf_feat_params(adapter, &get_feat_ctx);
>  
> -	adapter->msg_enable = netif_msg_init(debug, DEFAULT_MSG_ENABLE);
>  	adapter->reset_reason = ENA_REGS_RESET_NORMAL;
>  
>  	adapter->requested_tx_ring_size = calc_queue_ctx.tx_queue_size;
> @@ -4257,7 +4257,7 @@ static int ena_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>  	if (rc) {
>  		dev_err(&pdev->dev,
>  			"Failed to query interrupt moderation feature\n");
> -		goto err_netdev_destroy;
> +		goto err_device_destroy;
>  	}
>  	ena_init_io_rings(adapter,
>  			  0,
> @@ -4335,11 +4335,11 @@ static int ena_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>  	ena_disable_msix(adapter);
>  err_worker_destroy:
>  	del_timer(&adapter->timer_service);
> -err_netdev_destroy:
> -	free_netdev(netdev);
>  err_device_destroy:
>  	ena_com_delete_host_info(ena_dev);
>  	ena_com_admin_destroy(ena_dev);
> +err_netdev_destroy:
> +	free_netdev(netdev);
>  err_free_region:
>  	ena_release_bars(ena_dev, pdev);
>  err_free_ena_dev:
>

Kiyanovski, Arthur Dec. 3, 2020, 2:38 p.m. UTC | #2

> -----Original Message-----
> From: Heiner Kallweit <[e-mail]>
> Sent: Wednesday, December 2, 2020 11:55 PM
> To: Kiyanovski, Arthur <[e-mail]>; [e-mail];
> [e-mail]
> Cc: Woodhouse, David <[e-mail]>; Machulsky, Zorik
> <[e-mail]>; Matushevsky, Alexander <[e-mail]>;
> Bshara, Saeed <[e-mail]>; Wilson, Matt <[e-mail]>;
> Liguori, Anthony <[e-mail]>; Bshara, Nafea
> <[e-mail]>; Tzalik, Guy <[e-mail]>; Belgazal,
> Netanel <[e-mail]>; Saidi, Ali <[e-mail]>;
> Herrenschmidt, Benjamin <[e-mail]>; Dagan, Noam
> <[e-mail]>; Agroskin, Shay <[e-mail]>; Jubran,
> Samih <[e-mail]>
> Subject: RE: [EXTERNAL] [PATCH V3 net-next 1/9] net: ena: use constant
> value for net_device allocation
> 
> CAUTION: This email originated from outside of the organization. Do not click
> links or open attachments unless you can confirm the sender and know the
> content is safe.
> 
> 
> 
> Am 02.12.2020 um 21:03 schrieb [e-mail]:
> > From: Arthur Kiyanovski <[e-mail]>
> >
> > The patch changes the maximum number of RX/TX queues it advertises to
> > the kernel (via alloc_etherdev_mq()) from a value received from the
> > device to a constant value which is the minimum between 128 and the
> > number of CPUs in the system.
> >
> > By allocating the net_device struct with a constant number of queues,
> > the driver is able to allocate it at a much earlier stage, before
> > calling any ena_com functions. This would allow to make all log prints
> > in ena_com to use netdev_* log functions instead or current pr_* ones.
> >
> 
> Did you test this? Usually using netdev_* before the net_device is registered
> results in quite ugly messages. Therefore there's a number of patches doing
> the opposite, replacing netdev_* with dev_* before register_netdev(). See
> e.g.
> 22148df0d0bd ("r8169: don't use netif_info et al before net_device has been
> registered")

Thanks for your comment.
Yes we did test it.
Please see the discussion which led to this patch in a previous thread here:
https://www.mail-archive.com/[e-mail]/msg353590.html
 
> > Signed-off-by: Shay Agroskin <[e-mail]>
> > Signed-off-by: Arthur Kiyanovski <[e-mail]>
> > ---
> >  drivers/net/ethernet/amazon/ena/ena_netdev.c | 46
> > ++++++++++----------
> >  1 file changed, 23 insertions(+), 23 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/amazon/ena/ena_netdev.c
> > b/drivers/net/ethernet/amazon/ena/ena_netdev.c
> > index df1884d57d1a..985dea1870b5 100644
> > --- a/drivers/net/ethernet/amazon/ena/ena_netdev.c
> > +++ b/drivers/net/ethernet/amazon/ena/ena_netdev.c
> > @@ -29,6 +29,8 @@ MODULE_LICENSE("GPL");
> >  /* Time in jiffies before concluding the transmitter is hung. */
> > #define TX_TIMEOUT  (5 * HZ)
> >
> > +#define ENA_MAX_RINGS min_t(unsigned int,
> ENA_MAX_NUM_IO_QUEUES,
> > +num_possible_cpus())
> > +
> >  #define ENA_NAPI_BUDGET 64
> >
> >  #define DEFAULT_MSG_ENABLE (NETIF_MSG_DRV | NETIF_MSG_PROBE
> |
> > NETIF_MSG_IFUP | \ @@ -4176,18 +4178,34 @@ static int
> ena_probe(struct
> > pci_dev *pdev, const struct pci_device_id *ent)
> >
> >       ena_dev->dmadev = &pdev->dev;
> >
> > +     netdev = alloc_etherdev_mq(sizeof(struct ena_adapter),
> ENA_MAX_RINGS);
> > +     if (!netdev) {
> > +             dev_err(&pdev->dev, "alloc_etherdev_mq failed\n");
> > +             rc = -ENOMEM;
> > +             goto err_free_region;
> > +     }
> > +
> > +     SET_NETDEV_DEV(netdev, &pdev->dev);
> > +     adapter = netdev_priv(netdev);
> > +     adapter->ena_dev = ena_dev;
> > +     adapter->netdev = netdev;
> > +     adapter->pdev = pdev;
> > +     adapter->msg_enable = netif_msg_init(debug,
> DEFAULT_MSG_ENABLE);
> > +
> > +     pci_set_drvdata(pdev, adapter);
> > +
> >       rc = ena_device_init(ena_dev, pdev, &get_feat_ctx, &wd_state);
> >       if (rc) {
> >               dev_err(&pdev->dev, "ENA device init failed\n");
> >               if (rc == -ETIME)
> >                       rc = -EPROBE_DEFER;
> > -             goto err_free_region;
> > +             goto err_netdev_destroy;
> >       }
> >
> >       rc = ena_map_llq_mem_bar(pdev, ena_dev, bars);
> >       if (rc) {
> >               dev_err(&pdev->dev, "ENA llq bar mapping failed\n");
> > -             goto err_free_ena_dev;
> > +             goto err_device_destroy;
> >       }
> >
> >       calc_queue_ctx.ena_dev = ena_dev; @@ -4207,26 +4225,8 @@ static
> > int ena_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
> >               goto err_device_destroy;
> >       }
> >
> > -     /* dev zeroed in init_etherdev */
> > -     netdev = alloc_etherdev_mq(sizeof(struct ena_adapter),
> max_num_io_queues);
> > -     if (!netdev) {
> > -             dev_err(&pdev->dev, "alloc_etherdev_mq failed\n");
> > -             rc = -ENOMEM;
> > -             goto err_device_destroy;
> > -     }
> > -
> > -     SET_NETDEV_DEV(netdev, &pdev->dev);
> > -
> > -     adapter = netdev_priv(netdev);
> > -     pci_set_drvdata(pdev, adapter);
> > -
> > -     adapter->ena_dev = ena_dev;
> > -     adapter->netdev = netdev;
> > -     adapter->pdev = pdev;
> > -
> >       ena_set_conf_feat_params(adapter, &get_feat_ctx);
> >
> > -     adapter->msg_enable = netif_msg_init(debug,
> DEFAULT_MSG_ENABLE);
> >       adapter->reset_reason = ENA_REGS_RESET_NORMAL;
> >
> >       adapter->requested_tx_ring_size = calc_queue_ctx.tx_queue_size;
> > @@ -4257,7 +4257,7 @@ static int ena_probe(struct pci_dev *pdev, const
> struct pci_device_id *ent)
> >       if (rc) {
> >               dev_err(&pdev->dev,
> >                       "Failed to query interrupt moderation feature\n");
> > -             goto err_netdev_destroy;
> > +             goto err_device_destroy;
> >       }
> >       ena_init_io_rings(adapter,
> >                         0,
> > @@ -4335,11 +4335,11 @@ static int ena_probe(struct pci_dev *pdev,
> const struct pci_device_id *ent)
> >       ena_disable_msix(adapter);
> >  err_worker_destroy:
> >       del_timer(&adapter->timer_service);
> > -err_netdev_destroy:
> > -     free_netdev(netdev);
> >  err_device_destroy:
> >       ena_com_delete_host_info(ena_dev);
> >       ena_com_admin_destroy(ena_dev);
> > +err_netdev_destroy:
> > +     free_netdev(netdev);
> >  err_free_region:
> >       ena_release_bars(ena_dev, pdev);
> >  err_free_ena_dev:
> >

Heiner Kallweit Dec. 3, 2020, 8:56 p.m. UTC | #3

Am 03.12.2020 um 15:38 schrieb Kiyanovski, Arthur:
> 
> 
>> -----Original Message-----
>> From: Heiner Kallweit <[e-mail]>
>> Sent: Wednesday, December 2, 2020 11:55 PM
>> To: Kiyanovski, Arthur <[e-mail]>; [e-mail];
>> [e-mail]
>> Cc: Woodhouse, David <[e-mail]>; Machulsky, Zorik
>> <[e-mail]>; Matushevsky, Alexander <[e-mail]>;
>> Bshara, Saeed <[e-mail]>; Wilson, Matt <[e-mail]>;
>> Liguori, Anthony <[e-mail]>; Bshara, Nafea
>> <[e-mail]>; Tzalik, Guy <[e-mail]>; Belgazal,
>> Netanel <[e-mail]>; Saidi, Ali <[e-mail]>;
>> Herrenschmidt, Benjamin <[e-mail]>; Dagan, Noam
>> <[e-mail]>; Agroskin, Shay <[e-mail]>; Jubran,
>> Samih <[e-mail]>
>> Subject: RE: [EXTERNAL] [PATCH V3 net-next 1/9] net: ena: use constant
>> value for net_device allocation
>>
>> CAUTION: This email originated from outside of the organization. Do not click
>> links or open attachments unless you can confirm the sender and know the
>> content is safe.
>>
>>
>>
>> Am 02.12.2020 um 21:03 schrieb [e-mail]:
>>> From: Arthur Kiyanovski <[e-mail]>
>>>
>>> The patch changes the maximum number of RX/TX queues it advertises to
>>> the kernel (via alloc_etherdev_mq()) from a value received from the
>>> device to a constant value which is the minimum between 128 and the
>>> number of CPUs in the system.
>>>
>>> By allocating the net_device struct with a constant number of queues,
>>> the driver is able to allocate it at a much earlier stage, before
>>> calling any ena_com functions. This would allow to make all log prints
>>> in ena_com to use netdev_* log functions instead or current pr_* ones.
>>>
>>
>> Did you test this? Usually using netdev_* before the net_device is registered
>> results in quite ugly messages. Therefore there's a number of patches doing
>> the opposite, replacing netdev_* with dev_* before register_netdev(). See
>> e.g.
>> 22148df0d0bd ("r8169: don't use netif_info et al before net_device has been
>> registered")
> 
> Thanks for your comment.
> Yes we did test it.
> Please see the discussion which led to this patch in a previous thread here:
> https://www.mail-archive.com/[e-mail]/msg353590.html
>  

Ah, I see. After reading the mail thread your motivation is clear.
You accept ugly messages when ena_com functions are called from probe()
for the sake of better messages when the same ena_com functions are
called later from other parts of the driver. Maybe an explanation of
this tradeoff would have been good in the commit message (or a link
to the mail thread).

>>> Signed-off-by: Shay Agroskin <[e-mail]>
>>> Signed-off-by: Arthur Kiyanovski <[e-mail]>
>>> ---
>>>  drivers/net/ethernet/amazon/ena/ena_netdev.c | 46
>>> ++++++++++----------
>>>  1 file changed, 23 insertions(+), 23 deletions(-)
>>>
>>> diff --git a/drivers/net/ethernet/amazon/ena/ena_netdev.c
>>> b/drivers/net/ethernet/amazon/ena/ena_netdev.c
>>> index df1884d57d1a..985dea1870b5 100644
>>> --- a/drivers/net/ethernet/amazon/ena/ena_netdev.c
>>> +++ b/drivers/net/ethernet/amazon/ena/ena_netdev.c
>>> @@ -29,6 +29,8 @@ MODULE_LICENSE("GPL");
>>>  /* Time in jiffies before concluding the transmitter is hung. */
>>> #define TX_TIMEOUT  (5 * HZ)
>>>
>>> +#define ENA_MAX_RINGS min_t(unsigned int,
>> ENA_MAX_NUM_IO_QUEUES,
>>> +num_possible_cpus())
>>> +
>>>  #define ENA_NAPI_BUDGET 64
>>>
>>>  #define DEFAULT_MSG_ENABLE (NETIF_MSG_DRV | NETIF_MSG_PROBE
>> |
>>> NETIF_MSG_IFUP | \ @@ -4176,18 +4178,34 @@ static int
>> ena_probe(struct
>>> pci_dev *pdev, const struct pci_device_id *ent)
>>>
>>>       ena_dev->dmadev = &pdev->dev;
>>>
>>> +     netdev = alloc_etherdev_mq(sizeof(struct ena_adapter),
>> ENA_MAX_RINGS);
>>> +     if (!netdev) {
>>> +             dev_err(&pdev->dev, "alloc_etherdev_mq failed\n");
>>> +             rc = -ENOMEM;
>>> +             goto err_free_region;
>>> +     }
>>> +
>>> +     SET_NETDEV_DEV(netdev, &pdev->dev);
>>> +     adapter = netdev_priv(netdev);
>>> +     adapter->ena_dev = ena_dev;
>>> +     adapter->netdev = netdev;
>>> +     adapter->pdev = pdev;
>>> +     adapter->msg_enable = netif_msg_init(debug,
>> DEFAULT_MSG_ENABLE);
>>> +
>>> +     pci_set_drvdata(pdev, adapter);
>>> +
>>>       rc = ena_device_init(ena_dev, pdev, &get_feat_ctx, &wd_state);
>>>       if (rc) {
>>>               dev_err(&pdev->dev, "ENA device init failed\n");
>>>               if (rc == -ETIME)
>>>                       rc = -EPROBE_DEFER;
>>> -             goto err_free_region;
>>> +             goto err_netdev_destroy;
>>>       }
>>>
>>>       rc = ena_map_llq_mem_bar(pdev, ena_dev, bars);
>>>       if (rc) {
>>>               dev_err(&pdev->dev, "ENA llq bar mapping failed\n");
>>> -             goto err_free_ena_dev;
>>> +             goto err_device_destroy;
>>>       }
>>>
>>>       calc_queue_ctx.ena_dev = ena_dev; @@ -4207,26 +4225,8 @@ static
>>> int ena_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>>>               goto err_device_destroy;
>>>       }
>>>
>>> -     /* dev zeroed in init_etherdev */
>>> -     netdev = alloc_etherdev_mq(sizeof(struct ena_adapter),
>> max_num_io_queues);
>>> -     if (!netdev) {
>>> -             dev_err(&pdev->dev, "alloc_etherdev_mq failed\n");
>>> -             rc = -ENOMEM;
>>> -             goto err_device_destroy;
>>> -     }
>>> -
>>> -     SET_NETDEV_DEV(netdev, &pdev->dev);
>>> -
>>> -     adapter = netdev_priv(netdev);
>>> -     pci_set_drvdata(pdev, adapter);
>>> -
>>> -     adapter->ena_dev = ena_dev;
>>> -     adapter->netdev = netdev;
>>> -     adapter->pdev = pdev;
>>> -
>>>       ena_set_conf_feat_params(adapter, &get_feat_ctx);
>>>
>>> -     adapter->msg_enable = netif_msg_init(debug,
>> DEFAULT_MSG_ENABLE);
>>>       adapter->reset_reason = ENA_REGS_RESET_NORMAL;
>>>
>>>       adapter->requested_tx_ring_size = calc_queue_ctx.tx_queue_size;
>>> @@ -4257,7 +4257,7 @@ static int ena_probe(struct pci_dev *pdev, const
>> struct pci_device_id *ent)
>>>       if (rc) {
>>>               dev_err(&pdev->dev,
>>>                       "Failed to query interrupt moderation feature\n");
>>> -             goto err_netdev_destroy;
>>> +             goto err_device_destroy;
>>>       }
>>>       ena_init_io_rings(adapter,
>>>                         0,
>>> @@ -4335,11 +4335,11 @@ static int ena_probe(struct pci_dev *pdev,
>> const struct pci_device_id *ent)
>>>       ena_disable_msix(adapter);
>>>  err_worker_destroy:
>>>       del_timer(&adapter->timer_service);
>>> -err_netdev_destroy:
>>> -     free_netdev(netdev);
>>>  err_device_destroy:
>>>       ena_com_delete_host_info(ena_dev);
>>>       ena_com_admin_destroy(ena_dev);
>>> +err_netdev_destroy:
>>> +     free_netdev(netdev);
>>>  err_free_region:
>>>       ena_release_bars(ena_dev, pdev);
>>>  err_free_ena_dev:
>>>
>

Kiyanovski, Arthur Dec. 4, 2020, 12:15 p.m. UTC | #4

> -----Original Message-----
> From: Heiner Kallweit <[e-mail]>
> Sent: Thursday, December 3, 2020 10:56 PM
> To: Kiyanovski, Arthur <[e-mail]>; [e-mail];
> [e-mail]; [e-mail]
> Cc: Woodhouse, David <[e-mail]>; Machulsky, Zorik
> <[e-mail]>; Matushevsky, Alexander <[e-mail]>;
> Bshara, Saeed <[e-mail]>; Wilson, Matt <[e-mail]>;
> Liguori, Anthony <[e-mail]>; Bshara, Nafea
> <[e-mail]>; Tzalik, Guy <[e-mail]>; Belgazal,
> Netanel <[e-mail]>; Saidi, Ali <[e-mail]>;
> Herrenschmidt, Benjamin <[e-mail]>; Dagan, Noam
> <[e-mail]>; Agroskin, Shay <[e-mail]>; Jubran,
> Samih <[e-mail]>
> Subject: RE: [EXTERNAL] [PATCH V3 net-next 1/9] net: ena: use constant
> value for net_device allocation
> 
> CAUTION: This email originated from outside of the organization. Do not click
> links or open attachments unless you can confirm the sender and know the
> content is safe.
> 
> 
> 
> Am 03.12.2020 um 15:38 schrieb Kiyanovski, Arthur:
> >
> >
> >> -----Original Message-----
> >> From: Heiner Kallweit <[e-mail]>
> >> Sent: Wednesday, December 2, 2020 11:55 PM
> >> To: Kiyanovski, Arthur <[e-mail]>; [e-mail];
> >> [e-mail]
> >> Cc: Woodhouse, David <[e-mail]>; Machulsky, Zorik
> >> <[e-mail]>; Matushevsky, Alexander <[e-mail]>;
> >> Bshara, Saeed <[e-mail]>; Wilson, Matt
> <[e-mail]>;
> >> Liguori, Anthony <[e-mail]>; Bshara, Nafea
> >> <[e-mail]>; Tzalik, Guy <[e-mail]>; Belgazal,
> >> Netanel <[e-mail]>; Saidi, Ali <[e-mail]>;
> >> Herrenschmidt, Benjamin <[e-mail]>; Dagan, Noam
> >> <[e-mail]>; Agroskin, Shay <[e-mail]>;
> Jubran,
> >> Samih <[e-mail]>
> >> Subject: RE: [EXTERNAL] [PATCH V3 net-next 1/9] net: ena: use
> >> constant value for net_device allocation
> >>
> >> CAUTION: This email originated from outside of the organization. Do
> >> not click links or open attachments unless you can confirm the sender
> >> and know the content is safe.
> >>
> >>
> >>
> >> Am 02.12.2020 um 21:03 schrieb [e-mail]:
> >>> From: Arthur Kiyanovski <[e-mail]>
> >>>
> >>> The patch changes the maximum number of RX/TX queues it advertises
> >>> to the kernel (via alloc_etherdev_mq()) from a value received from
> >>> the device to a constant value which is the minimum between 128 and
> >>> the number of CPUs in the system.
> >>>
> >>> By allocating the net_device struct with a constant number of
> >>> queues, the driver is able to allocate it at a much earlier stage,
> >>> before calling any ena_com functions. This would allow to make all
> >>> log prints in ena_com to use netdev_* log functions instead or current
> pr_* ones.
> >>>
> >>
> >> Did you test this? Usually using netdev_* before the net_device is
> >> registered results in quite ugly messages. Therefore there's a number
> >> of patches doing the opposite, replacing netdev_* with dev_* before
> >> register_netdev(). See e.g.
> >> 22148df0d0bd ("r8169: don't use netif_info et al before net_device
> >> has been
> >> registered")
> >
> > Thanks for your comment.
> > Yes we did test it.
> > Please see the discussion which led to this patch in a previous thread here:
> > https://www.mail-archive.com/[e-mail]/msg353590.html
> >
> 
> Ah, I see. After reading the mail thread your motivation is clear.
> You accept ugly messages when ena_com functions are called from probe()
> for the sake of better messages when the same ena_com functions are
> called later from other parts of the driver. Maybe an explanation of this
> tradeoff would have been good in the commit message (or a link to the mail
> thread).

Good idea.
Added this explanation to the next version of this patchset.
Thanks!

> >>> Signed-off-by: Shay Agroskin <[e-mail]>
> >>> Signed-off-by: Arthur Kiyanovski <[e-mail]>
> >>> ---
> >>>  drivers/net/ethernet/amazon/ena/ena_netdev.c | 46
> >>> ++++++++++----------
> >>>  1 file changed, 23 insertions(+), 23 deletions(-)
> >>>
> >>> diff --git a/drivers/net/ethernet/amazon/ena/ena_netdev.c
> >>> b/drivers/net/ethernet/amazon/ena/ena_netdev.c
> >>> index df1884d57d1a..985dea1870b5 100644
> >>> --- a/drivers/net/ethernet/amazon/ena/ena_netdev.c
> >>> +++ b/drivers/net/ethernet/amazon/ena/ena_netdev.c
> >>> @@ -29,6 +29,8 @@ MODULE_LICENSE("GPL");
> >>>  /* Time in jiffies before concluding the transmitter is hung. */
> >>> #define TX_TIMEOUT  (5 * HZ)
> >>>
> >>> +#define ENA_MAX_RINGS min_t(unsigned int,
> >> ENA_MAX_NUM_IO_QUEUES,
> >>> +num_possible_cpus())
> >>> +
> >>>  #define ENA_NAPI_BUDGET 64
> >>>
> >>>  #define DEFAULT_MSG_ENABLE (NETIF_MSG_DRV |
> NETIF_MSG_PROBE
> >> |
> >>> NETIF_MSG_IFUP | \ @@ -4176,18 +4178,34 @@ static int
> >> ena_probe(struct
> >>> pci_dev *pdev, const struct pci_device_id *ent)
> >>>
> >>>       ena_dev->dmadev = &pdev->dev;
> >>>
> >>> +     netdev = alloc_etherdev_mq(sizeof(struct ena_adapter),
> >> ENA_MAX_RINGS);
> >>> +     if (!netdev) {
> >>> +             dev_err(&pdev->dev, "alloc_etherdev_mq failed\n");
> >>> +             rc = -ENOMEM;
> >>> +             goto err_free_region;
> >>> +     }
> >>> +
> >>> +     SET_NETDEV_DEV(netdev, &pdev->dev);
> >>> +     adapter = netdev_priv(netdev);
> >>> +     adapter->ena_dev = ena_dev;
> >>> +     adapter->netdev = netdev;
> >>> +     adapter->pdev = pdev;
> >>> +     adapter->msg_enable = netif_msg_init(debug,
> >> DEFAULT_MSG_ENABLE);
> >>> +
> >>> +     pci_set_drvdata(pdev, adapter);
> >>> +
> >>>       rc = ena_device_init(ena_dev, pdev, &get_feat_ctx, &wd_state);
> >>>       if (rc) {
> >>>               dev_err(&pdev->dev, "ENA device init failed\n");
> >>>               if (rc == -ETIME)
> >>>                       rc = -EPROBE_DEFER;
> >>> -             goto err_free_region;
> >>> +             goto err_netdev_destroy;
> >>>       }
> >>>
> >>>       rc = ena_map_llq_mem_bar(pdev, ena_dev, bars);
> >>>       if (rc) {
> >>>               dev_err(&pdev->dev, "ENA llq bar mapping failed\n");
> >>> -             goto err_free_ena_dev;
> >>> +             goto err_device_destroy;
> >>>       }
> >>>
> >>>       calc_queue_ctx.ena_dev = ena_dev; @@ -4207,26 +4225,8 @@
> >>> static int ena_probe(struct pci_dev *pdev, const struct pci_device_id
> *ent)
> >>>               goto err_device_destroy;
> >>>       }
> >>>
> >>> -     /* dev zeroed in init_etherdev */
> >>> -     netdev = alloc_etherdev_mq(sizeof(struct ena_adapter),
> >> max_num_io_queues);
> >>> -     if (!netdev) {
> >>> -             dev_err(&pdev->dev, "alloc_etherdev_mq failed\n");
> >>> -             rc = -ENOMEM;
> >>> -             goto err_device_destroy;
> >>> -     }
> >>> -
> >>> -     SET_NETDEV_DEV(netdev, &pdev->dev);
> >>> -
> >>> -     adapter = netdev_priv(netdev);
> >>> -     pci_set_drvdata(pdev, adapter);
> >>> -
> >>> -     adapter->ena_dev = ena_dev;
> >>> -     adapter->netdev = netdev;
> >>> -     adapter->pdev = pdev;
> >>> -
> >>>       ena_set_conf_feat_params(adapter, &get_feat_ctx);
> >>>
> >>> -     adapter->msg_enable = netif_msg_init(debug,
> >> DEFAULT_MSG_ENABLE);
> >>>       adapter->reset_reason = ENA_REGS_RESET_NORMAL;
> >>>
> >>>       adapter->requested_tx_ring_size =
> >>> calc_queue_ctx.tx_queue_size; @@ -4257,7 +4257,7 @@ static int
> >>> ena_probe(struct pci_dev *pdev, const
> >> struct pci_device_id *ent)
> >>>       if (rc) {
> >>>               dev_err(&pdev->dev,
> >>>                       "Failed to query interrupt moderation feature\n");
> >>> -             goto err_netdev_destroy;
> >>> +             goto err_device_destroy;
> >>>       }
> >>>       ena_init_io_rings(adapter,
> >>>                         0,
> >>> @@ -4335,11 +4335,11 @@ static int ena_probe(struct pci_dev *pdev,
> >> const struct pci_device_id *ent)
> >>>       ena_disable_msix(adapter);
> >>>  err_worker_destroy:
> >>>       del_timer(&adapter->timer_service);
> >>> -err_netdev_destroy:
> >>> -     free_netdev(netdev);
> >>>  err_device_destroy:
> >>>       ena_com_delete_host_info(ena_dev);
> >>>       ena_com_admin_destroy(ena_dev);
> >>> +err_netdev_destroy:
> >>> +     free_netdev(netdev);
> >>>  err_free_region:
> >>>       ena_release_bars(ena_dev, pdev);
> >>>  err_free_ena_dev:
> >>>
> >

Patch

diff --git a/drivers/net/ethernet/amazon/ena/ena_netdev.c b/drivers/net/ethernet/amazon/ena/ena_netdev.c
index df1884d57d1a..985dea1870b5 100644
--- a/drivers/net/ethernet/amazon/ena/ena_netdev.c
+++ b/drivers/net/ethernet/amazon/ena/ena_netdev.c
@@ -29,6 +29,8 @@  MODULE_LICENSE("GPL");
 /* Time in jiffies before concluding the transmitter is hung. */
 #define TX_TIMEOUT  (5 * HZ)
 
+#define ENA_MAX_RINGS min_t(unsigned int, ENA_MAX_NUM_IO_QUEUES, num_possible_cpus())
+
 #define ENA_NAPI_BUDGET 64
 
 #define DEFAULT_MSG_ENABLE (NETIF_MSG_DRV | NETIF_MSG_PROBE | NETIF_MSG_IFUP | \
@@ -4176,18 +4178,34 @@  static int ena_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 
 	ena_dev->dmadev = &pdev->dev;
 
+	netdev = alloc_etherdev_mq(sizeof(struct ena_adapter), ENA_MAX_RINGS);
+	if (!netdev) {
+		dev_err(&pdev->dev, "alloc_etherdev_mq failed\n");
+		rc = -ENOMEM;
+		goto err_free_region;
+	}
+
+	SET_NETDEV_DEV(netdev, &pdev->dev);
+	adapter = netdev_priv(netdev);
+	adapter->ena_dev = ena_dev;
+	adapter->netdev = netdev;
+	adapter->pdev = pdev;
+	adapter->msg_enable = netif_msg_init(debug, DEFAULT_MSG_ENABLE);
+
+	pci_set_drvdata(pdev, adapter);
+
 	rc = ena_device_init(ena_dev, pdev, &get_feat_ctx, &wd_state);
 	if (rc) {
 		dev_err(&pdev->dev, "ENA device init failed\n");
 		if (rc == -ETIME)
 			rc = -EPROBE_DEFER;
-		goto err_free_region;
+		goto err_netdev_destroy;
 	}
 
 	rc = ena_map_llq_mem_bar(pdev, ena_dev, bars);
 	if (rc) {
 		dev_err(&pdev->dev, "ENA llq bar mapping failed\n");
-		goto err_free_ena_dev;
+		goto err_device_destroy;
 	}
 
 	calc_queue_ctx.ena_dev = ena_dev;
@@ -4207,26 +4225,8 @@  static int ena_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 		goto err_device_destroy;
 	}
 
-	/* dev zeroed in init_etherdev */
-	netdev = alloc_etherdev_mq(sizeof(struct ena_adapter), max_num_io_queues);
-	if (!netdev) {
-		dev_err(&pdev->dev, "alloc_etherdev_mq failed\n");
-		rc = -ENOMEM;
-		goto err_device_destroy;
-	}
-
-	SET_NETDEV_DEV(netdev, &pdev->dev);
-
-	adapter = netdev_priv(netdev);
-	pci_set_drvdata(pdev, adapter);
-
-	adapter->ena_dev = ena_dev;
-	adapter->netdev = netdev;
-	adapter->pdev = pdev;
-
 	ena_set_conf_feat_params(adapter, &get_feat_ctx);
 
-	adapter->msg_enable = netif_msg_init(debug, DEFAULT_MSG_ENABLE);
 	adapter->reset_reason = ENA_REGS_RESET_NORMAL;
 
 	adapter->requested_tx_ring_size = calc_queue_ctx.tx_queue_size;
@@ -4257,7 +4257,7 @@  static int ena_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 	if (rc) {
 		dev_err(&pdev->dev,
 			"Failed to query interrupt moderation feature\n");
-		goto err_netdev_destroy;
+		goto err_device_destroy;
 	}
 	ena_init_io_rings(adapter,
 			  0,
@@ -4335,11 +4335,11 @@  static int ena_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 	ena_disable_msix(adapter);
 err_worker_destroy:
 	del_timer(&adapter->timer_service);
-err_netdev_destroy:
-	free_netdev(netdev);
 err_device_destroy:
 	ena_com_delete_host_info(ena_dev);
 	ena_com_admin_destroy(ena_dev);
+err_netdev_destroy:
+	free_netdev(netdev);
 err_free_region:
 	ena_release_bars(ena_dev, pdev);
 err_free_ena_dev: