Re: [dpdk-dev] [PATCH 11/12] net/vhost: support to run in the secondary process

[Yuanhan Liu <yliu@fridaylinux.org>]

On Fri, Aug 25, 2017 at 09:40:51AM +0000, Jianfeng Tan wrote:
>  static int
> +share_device(int vid)
> +{
> +	uint32_t i, vring_num;
> +	int len;
> +	int fds[8];
> +	struct rte_vhost_memory *mem;
> +	struct vhost_params *params;
> +	struct rte_vhost_vring vring;
> +
> +	/* share mem table */
> +	if (rte_vhost_get_mem_table(vid, &mem) < 0) {
> +		printf("Failed to get mem table\n");

No printf in DPDK lib and pmd, use RTE_LOG instead.

[...]
> +static int
> +eth_dev_vhost_attach(struct rte_vdev_device *dev)
> +{
> +	struct rte_eth_dev *eth_dev = NULL;
> +	struct rte_eth_dev_data *data = NULL;
> +
> +	RTE_LOG(INFO, PMD, "Attach vhost user port\n");
> +
> +	/* reserve an ethdev entry */
> +	eth_dev = rte_eth_vdev_allocate(dev, sizeof(struct pmd_internal));
> +	if (eth_dev == NULL)
> +		goto error;

I'd suggest to "return -1" directly here, without introducing goto.

> +
> +	eth_dev->dev_ops = &ops;
> +
> +	/* finally assign rx and tx ops */
> +	eth_dev->rx_pkt_burst = eth_vhost_rx;
> +	eth_dev->tx_pkt_burst = eth_vhost_tx;
> +
> +	data = eth_dev->data;
> +
> +	return data->port_id;
> +
> +error:
> +	return -1;
> +}
> +
>  static inline int
>  open_iface(const char *key __rte_unused, const char *value, void *extra_args)
>  {
> @@ -1154,6 +1244,39 @@ open_int(const char *key __rte_unused, const char *value, void *extra_args)
>  }
>  
>  static int
> +vhost_pmd_action(const char *params, int len __rte_unused,
> +		 int fds[], int fds_num)
> +{
> +	int i;
> +	void *base_addr;
> +	const struct vhost_params *p = (const struct vhost_params *)params;

The cast could be avoided if you define the action prototype with
"const void *params".

> +	const struct rte_vhost_mem_region *regions;
> +
> +	switch (p->type) {
> +	case VHOST_MSG_TYPE_REGIONS:
> +		regions = (const void *)p->regions;

Unnecessary cast.

> +		for (i = 0; i < fds_num; ++i) {
> +			base_addr = mmap(regions[i].mmap_addr,
> +					 regions[i].mmap_size,
> +					 PROT_READ | PROT_WRITE,
> +					 MAP_FIXED | MAP_SHARED, fds[i], 0);
> +			if (base_addr != regions[i].mmap_addr) {
> +				RTE_LOG(INFO, PMD, "mmap error");

The log level should be error, nothing will work if it happens. Also, the
log message should be more informative.

	--yliu