> -----Original Message----- > From: Heiner Kallweit <[email protected]> > Sent: Thursday, December 3, 2020 10:56 PM > To: Kiyanovski, Arthur <[email protected]>; [email protected]; > [email protected]; [email protected] > Cc: Woodhouse, David <[email protected]>; Machulsky, Zorik > <[email protected]>; Matushevsky, Alexander <[email protected]>; > Bshara, Saeed <[email protected]>; Wilson, Matt <[email protected]>; > Liguori, Anthony <[email protected]>; Bshara, Nafea > <[email protected]>; Tzalik, Guy <[email protected]>; Belgazal, > Netanel <[email protected]>; Saidi, Ali <[email protected]>; > Herrenschmidt, Benjamin <[email protected]>; Dagan, Noam > <[email protected]>; Agroskin, Shay <[email protected]>; Jubran, > Samih <[email protected]> > 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 <[email protected]> > >> Sent: Wednesday, December 2, 2020 11:55 PM > >> To: Kiyanovski, Arthur <[email protected]>; [email protected]; > >> [email protected] > >> Cc: Woodhouse, David <[email protected]>; Machulsky, Zorik > >> <[email protected]>; Matushevsky, Alexander <[email protected]>; > >> Bshara, Saeed <[email protected]>; Wilson, Matt > <[email protected]>; > >> Liguori, Anthony <[email protected]>; Bshara, Nafea > >> <[email protected]>; Tzalik, Guy <[email protected]>; Belgazal, > >> Netanel <[email protected]>; Saidi, Ali <[email protected]>; > >> Herrenschmidt, Benjamin <[email protected]>; Dagan, Noam > >> <[email protected]>; Agroskin, Shay <[email protected]>; > Jubran, > >> Samih <[email protected]> > >> 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 [email protected]: > >>> From: Arthur Kiyanovski <[email protected]> > >>> > >>> 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/[email protected]/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 <[email protected]> > >>> Signed-off-by: Arthur Kiyanovski <[email protected]> > >>> --- > >>> 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: > >>> > >
