Re: [spp] [PATCH v2 01/12] drivers/vhost: add multi process supported vhost PMD for SPP

[Itsuro ODA <oda@valinux.co.jp>]

Hi Yasufumi,

On Tue, 7 Jan 2020 19:41:52 +0900
Yasufumi Ogawa <yasufum.o@gmail.com> wrote:

> Hi,
> 
> On 2019/12/25 13:49, Itsuro Oda wrote:
> > vhost PMD can not be used by secondary processes since DPDK 18.11.
> > SPP project decided to have own vhost PMD which can be used by
> > secondary processes at the moment. This vhost PMD is based on the
> > original vhost PMD but is simplified very much only to support
> > functions used by SPP. Thereby it becomes easy to fix the probrem.
> >
> > The main idea of the fix is that execution of vhost start/stop
> > is moved to eth_dev_start/stop from probe/remove.
> >
> > Note that only process which executes eth_dev_start can use the
> > vhost device although the vhost device is shared among the primary
> > process and secondary processes. Once eth_dev_stop is executed by
> > the process which used the vhost device, it is available to be
> > used by any process. It is user responsibility that multipul
> > processes don't use the vhost device at the same time.
> >
> > Signed-off-by: Itsuro Oda <oda@valinux.co.jp>
> > ---
> >   src/drivers/vhost/Makefile                    |  28 +
> >   .../vhost/rte_pmd_spp_vhost_version.map       |   4 +
> >   src/drivers/vhost/rte_spp_vhost.c             | 588 ++++++++++++++++++
> >   3 files changed, 620 insertions(+)
> >   create mode 100644 src/drivers/vhost/Makefile
> >   create mode 100644 src/drivers/vhost/rte_pmd_spp_vhost_version.map
> >   create mode 100644 src/drivers/vhost/rte_spp_vhost.c
> >
> > diff --git a/src/drivers/vhost/Makefile b/src/drivers/vhost/Makefile
> [...]
> > +
> > +static inline struct pmd_internal *
> > +find_internal_resource(int vid)
> > +{
> > +	struct pmd_internal *internal;
> > +	int i;
> > +	char ifname[PATH_MAX];
> This variable is declared, but not used from anywhere.
> 
> > +
> > +	if (rte_vhost_get_ifname(vid, ifname, sizeof(ifname)) == -1)
> > +		return NULL;
> [...]

rte_vhost_get_ifname stores interface name to ifname.
ifname is compared with internal->ifname a few after lines.
"if (internal != NULL && !strcmp(internal->iface_name, ifname)) " 

> > +}
> > +
> > +static const struct eth_dev_ops ops = {
> > +	.dev_start = eth_dev_start,
> > +	.dev_stop = eth_dev_stop,
> > +	.dev_configure = eth_dev_configure,
> > +	.dev_infos_get = eth_dev_info, > +	.rx_queue_setup = eth_rx_queue_setup,
> > +	.tx_queue_setup = eth_tx_queue_setup,
> > +	.rx_queue_release = eth_queue_release,
> > +	.tx_queue_release = eth_queue_release,
> > +	.link_update = eth_link_update,
> > +	.stats_get = eth_stats_get,
> > +	.stats_reset = eth_stats_reset,
> > +};
> This struct causes compilation errors because of incompatible pointer types.
> 
> $ make
> == src
> == drivers
> == vhost
>    CC rte_spp_vhost.o
> rte_spp_vhost.c:371:19: error: initialization from incompatible pointer type [-Werror=incompatible-pointer-types]
>    .dev_infos_get = eth_dev_info,
>                     ^~~~~~~~~~~~
> rte_spp_vhost.c:371:19: note: (near initialization for ^[$B!F^[(Bops.dev_infos_get^[$B!G^[(B)
> rte_spp_vhost.c:378:17: error: initialization from incompatible pointer type [-Werror=incompatible-pointer-types]
>    .stats_reset = eth_stats_reset,
>                   ^~~~~~~~~~~~~~~
> rte_spp_vhost.c:378:17: note: (near initialization for ^[$B!F^[(Bops.stats_reset^[$B!G^[(B)
> cc1: error: unrecognized command line option ^[$B!F^[(B-Wno-address-of-packed-member^[$B!G^[(B [-Werror]
> cc1: all warnings being treated as errors
> 
>  From my understanding, eth_dev_info() and eth_stats_reset() are defined as void in your patch, but DPDK expects int by referring the definition of eth_dev_ops in lib/librte_ethdev/rte_ethdev_core.h:609.

The API of these methods are diffrent from v18.08 and v18.11 (or after).
You seem to make under v18.11 (or after). The patch is under v18.08.
Make sure your RTE_SDK.

> Regards,
> Yasufumi

Thanks.

> > +
> > +static int
> [...]

-- 
Itsuro ODA <oda@valinux.co.jp>