Hi Ferruh,
> -----Original Message-----
> From: Yigit, Ferruh
> Sent: Thursday, December 13, 2018 11:14 PM
> To: Lu, Wenzhuo <wenzhuo...@intel.com>; dev@dpdk.org
> Cc: Yang, Qiming <qiming.y...@intel.com>; Li, Xiaoyun
> <xiaoyun...@intel.com>; Wu, Jingjing <jingjing...@intel.com>
> Subject: Re: [dpdk-dev] [PATCH v3 16/34] net/ice: support device
> initialization
>
> On 12/13/2018 2:39 AM, Lu, Wenzhuo wrote:
> > Hi Ferruh,
> >
> >> -----Original Message-----
> >> From: Yigit, Ferruh
> >> Sent: Thursday, December 13, 2018 2:18 AM
> >> To: Lu, Wenzhuo <wenzhuo...@intel.com>; dev@dpdk.org
> >> Cc: Yang, Qiming <qiming.y...@intel.com>; Li, Xiaoyun
> >> <xiaoyun...@intel.com>; Wu, Jingjing <jingjing...@intel.com>
> >> Subject: Re: [dpdk-dev] [PATCH v3 16/34] net/ice: support device
> >> initialization
> >>
> >> On 12/12/2018 6:59 AM, Wenzhuo Lu wrote:
> >>> Signed-off-by: Wenzhuo Lu <wenzhuo...@intel.com>
> >>> Signed-off-by: Qiming Yang <qiming.y...@intel.com>
> >>> Signed-off-by: Xiaoyun Li <xiaoyun...@intel.com>
> >>> Signed-off-by: Jingjing Wu <jingjing...@intel.com>
> >>
> >> <...>
> >>
> >>> @@ -297,6 +297,15 @@
> >> CONFIG_RTE_LIBRTE_FM10K_RX_OLFLAGS_ENABLE=y
> >>> CONFIG_RTE_LIBRTE_FM10K_INC_VECTOR=y
> >>>
> >>> #
> >>> +# Compile burst-oriented ICE PMD driver #
> >> CONFIG_RTE_LIBRTE_ICE_PMD=y
> >>> +CONFIG_RTE_LIBRTE_ICE_DEBUG_RX=n
> >> CONFIG_RTE_LIBRTE_ICE_DEBUG_TX=n
> >>> +CONFIG_RTE_LIBRTE_ICE_DEBUG_TX_FREE=n
> >>> +CONFIG_RTE_LIBRTE_ICE_RX_ALLOW_BULK_ALLOC=y
> >>
> >> Is there a way to convert this into runtime config? Does it needs to
> >> be compile time config?
> > If only considering the functionality, we can totally remove this macro.
> That's why it's set to 'y' by default.
> > We introduce this macro for the users to improve performance for some
> specific cases. For example, if the MTU is small enough, we can totally
> remove the code wrapped by this macro. So the performance is better.
> Considering the purpose, to achieve the best performance, it's hard to make
> it a runtime configure.
>
> Thanks for clarification.
> >>
> >>> +
> >>> +include $(RTE_SDK)/mk/rte.lib.mk
> >>> diff --git a/drivers/net/ice/ice_ethdev.c
> >>> b/drivers/net/ice/ice_ethdev.c new file mode 100644 index
> >>> 0000000..e0bf15c
> >>> --- /dev/null
> >>> +++ b/drivers/net/ice/ice_ethdev.c
> >>> @@ -0,0 +1,640 @@
> >>> +/* SPDX-License-Identifier: BSD-3-Clause
> >>> + * Copyright(c) 2018 Intel Corporation */
> >>> +
> >>> +#include <rte_ethdev_pci.h>
> >>> +
> >>> +#include "base/ice_sched.h"
> >>> +#include "ice_ethdev.h"
> >>> +#include "ice_rxtx.h"
> >>> +
> >>> +#define ICE_MAX_QP_NUM "max_queue_pair_num"
> >>
> >> When documentation is added into this patch, can you also add this
> >> runtime config to that please?
> > The macro? It's not a configuration. It’s a string used internally.
>
> It is used as devargs string:
>
> + const char *queue_num_key = ICE_MAX_QP_NUM;
> <...>
> + if (!rte_kvargs_count(kvlist, queue_num_key)) {
> + rte_kvargs_free(kvlist);
> + return 0;
> + }
>
> And briefly what I am suggesting is to document runtime devargs in ice
> documentation.
Sorry, I didn’t get it at first. Will update the document.
>
> >
> >>
> >> <...>
> >>
> >>> +static int
> >>> +ice_dev_init(struct rte_eth_dev *dev) {
> >>> + struct rte_pci_device *pci_dev;
> >>> + struct ice_hw *hw = ICE_DEV_PRIVATE_TO_HW(dev->data-
> >>> dev_private);
> >>> + struct ice_pf *pf = ICE_DEV_PRIVATE_TO_PF(dev->data-
> >>> dev_private);
> >>> + int ret;
> >>> +
> >>> + dev->dev_ops = &ice_eth_dev_ops;
> >>> +
> >>> + pci_dev = RTE_DEV_TO_PCI(dev->device);
> >>> +
> >>> + rte_eth_copy_pci_info(dev, pci_dev);
> >>
> >> This is done by rte_eth_dev_pci_generic_probe(), do we need here?
> > No, we only need the info, don’t want to probe.
>
> Sorry, I didn't get it, let me rephrase:
>
> rte_eth_copy_pci_info() called as following with existing code:
> ice_pci_probe()
> rte_eth_dev_pci_generic_probe()
> rte_eth_dev_pci_allocate() <---- (1)
> rte_eth_copy_pci_info()
> ice_dev_init()
> rte_eth_copy_pci_info() <---- (2)
>
> I am asking can we remove (2) one, since (1) does the copy already?
Agree it looks redundant. Will check if we can remove (2).
>
> >
> >>
> >> <...>
> >>
> >>> +RTE_INIT(ice_init_log);
> >>> +static void
> >>> +ice_init_log(void)
> >>
> >> Can merge these lines, please check other samples.
> > Will change it in v4.
> >
> >>
> >>> +{
> >>> + ice_logtype_init = rte_log_register("pmd.ice.init");
> >>
> >> pmd.net.ice.init
> > Will change it in v4.
> >
> >>
> >>> + if (ice_logtype_init >= 0)
> >>> + rte_log_set_level(ice_logtype_init, RTE_LOG_NOTICE);
> >>> + ice_logtype_driver = rte_log_register("pmd.ice.driver");
> >>
> >> pmd.net.ice.driver
> > Will change it in v4.
> >
> >>
> >> <...>
> >>
> >>> +static void
> >>> +ice_dev_close(struct rte_eth_dev *dev) {
> >>> + struct ice_pf *pf = ICE_DEV_PRIVATE_TO_PF(dev->data-
> >>> dev_private);
> >>> + struct ice_hw *hw = ICE_DEV_PRIVATE_TO_HW(dev->data-
> >>> dev_private);
> >>> +
> >>> + ice_res_pool_destroy(&pf->msix_pool);
> >>> + ice_release_vsi(pf->main_vsi);
> >>> +
> >>> + ice_shutdown_all_ctrlq(hw);
> >>> +}
> >>
> >> I am mostly for ordering functions in a way that it doesn't require
> >> the forward declaration, which is mostly helps reading the code since
> >> the function order is close the call order.
> > I just want to align the order with ice_eth_dev_ops. But anyway I can move
> it forward.
>
> I think it make sense to align the order with ice_eth_dev_ops, again
> improves readability, and both can be done at same time:
>
> static dev_ops1() {}
> static dev_ops2() {}
> static dev_ops3() {}
>
> static const struct eth_dev_ops ice_eth_dev_ops = {
> ops1 = dev_ops1,
> ops2 = dev_ops2,
> ops3 = dev_ops3,
> };
>
> ice_dev_init() {
> dev->dev_ops = &ice_eth_dev_ops;
> }
>
> ice_pci_probe() {
> rte_eth_dev_pci_generic_probe(..., ice_dev_init); }
>
> rte_pci_driver rte_ice_pmd = {
> .probe = ice_pci_probe,
> .remove = ice_pci_remove,
> };
>
> RTE_PMD_REGISTER_PCI(net_ice, rte_ice_pmd);
>
> >
> >>
> >> It is up to you but also for sake of consistancy I think better to
> >> move this function up, and leave probe/remove/init_log functions as
> >> last functions in file.
> > So, it's a bad try to show the strong connection of probe/remove with
> > dev_init/uninit :)
>
> No it is good to show that relation and keep them close, sorry perhaps I
> missed your point but I wasn't suggesting to separate probe/remove &
> dev_init/uninit.
>
> It looks me better to keep file entry/exit points at the bottom and develop
> through upwards as call-stack moves deeper, instead of functions jump
> within the file against call-stack. This improves readability because you only
> scroll one way as you advance through code also you expose to code from
> more abstract to detailed order. That order also has benefit of removing
> forward declarations.
>
> I am aware it is already complex to develop a new PMD and you are already
> dealing with lots of hardware details, I have no intention to make it more
> complex for you, please take this only as an input.
I have the same feeling. Actually I've done much this kind of adjustment before
creating the patches. Will make it better.
