DPDK patches and discussions
 help / color / mirror / Atom feed
From: Ferruh Yigit <ferruh.yigit@intel.com>
To: Qi Zhang <qi.z.zhang@intel.com>
Cc: thomas@monjalon.net, dev@dpdk.org, xueqin.lin@intel.com
Subject: Re: [dpdk-dev] [PATCH v2] net/pcap: enable data path on secondary
Date: Tue, 13 Nov 2018 16:56:21 +0000
Message-ID: <861c43b2-a082-fdf8-61b9-2df1f4ed919c@intel.com> (raw)
In-Reply-To: <20181112165109.33296-1-qi.z.zhang@intel.com>

On 11/12/2018 4:51 PM, Qi Zhang wrote:
> Private vdev on secondary is never supported by the new shared
> device mode but pdump still relies on a private pcap PMD on secondary.
> The patch enables pcap PMD's data path on secondary so that pdump can
> work as usual.

It would be great if you described the problem a little more.

Private vdev was the way previously, when pdump developed, now with shared
device mode on virtual devices, pcap data path in secondary is not working.

What exactly not working is (please correct me if I am wrong):
When secondary adds a virtual device, related data transferred to primary and
primary creates the device and shares device back with secondary.
When pcap device created in primary, pcap handlers (pointers) are process local
and they are not valid for secondary process. This breaks secondary.

So we can't directly share the pcap handlers, but need to create a new set of
handlers for secondary, that is what you are doing in this patch, although I
have some comments, please check below.

Since there is single storage for pcap handlers that primary and secondary
shares and they can't share the handlers, you can't make both primary and
secondary data path work. Also freeing handlers is another concern. What is
needed is `rte_eth_dev->process_private` which has been added in this release.

> 
> Signed-off-by: Qi Zhang <qi.z.zhang@intel.com>
> Tested-by: Yufeng Mo <yufengx.mo@intel.com>

<...>

> @@ -934,6 +935,10 @@ pmd_init_internals(struct rte_vdev_device *vdev,
>  	 */
>  	(*eth_dev)->dev_ops = &ops;
>  
> +	/* store a copy of devargs for secondary process */
> +	strlcpy(internals->devargs, rte_vdev_device_args(vdev),
> +			ETH_PCAP_ARG_MAXLEN);

Why we need to cover this in PMD level?

Why secondary probe isn't getting devargs? Can't we fix this in eal level?
It can be OK to workaround in the PMD taking account of the time of the release,
but for long term I think this should be fixed in eal.

<...>

> @@ -1122,23 +1126,37 @@ pmd_pcap_probe(struct rte_vdev_device *dev)
>  	start_cycles = rte_get_timer_cycles();
>  	hz = rte_get_timer_hz();
>  
> -	if (rte_eal_process_type() == RTE_PROC_SECONDARY) {
> +	if (rte_eal_process_type() == RTE_PROC_PRIMARY) {
> +		kvlist = rte_kvargs_parse(rte_vdev_device_args(dev),
> +				valid_arguments);
> +		if (kvlist == NULL)
> +			return -EINVAL;
> +		if (rte_kvargs_count(kvlist, ETH_PCAP_IFACE_ARG) == 1)
> +			nb_rx_queue = 1;
> +		else
> +			nb_rx_queue =
> +				rte_kvargs_count(kvlist,
> +					ETH_PCAP_RX_PCAP_ARG) ? 1 : 0;
> +		nb_tx_queue = 1;

This part is wrong. pcap pmd supports multi queue, you can't hardcode the number
of queues. Also for Tx why it ignores `rx_iface` argument?
This is just hacking the driver for a specific use case breaking others.

> +		ret = pmd_init_internals(dev, nb_rx_queue,
> +				nb_tx_queue, &eth_dev);

I think it is not required to move pmd_init_internals() here.
This can be done simpler, I will send a draft patch as a reply to this mail for
possible solution.
But again that can't be final solution, we need to use `process_private`

<...>

  reply	other threads:[~2018-11-13 16:56 UTC|newest]

Thread overview: 23+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-11-05 21:08 [dpdk-dev] [PATCH] " Qi Zhang
2018-11-09 21:13 ` Ferruh Yigit
2018-11-09 21:24   ` Zhang, Qi Z
2018-11-12 16:51 ` [dpdk-dev] [PATCH v2] " Qi Zhang
2018-11-13 16:56   ` Ferruh Yigit [this message]
2018-11-13 17:11     ` [dpdk-dev] [PATCH] net/pcap: fix pcap handlers for secondary Ferruh Yigit
2018-11-13 17:14     ` [dpdk-dev] [PATCH v2] net/pcap: enable data path on secondary Thomas Monjalon
2018-11-13 18:27       ` Zhang, Qi Z
2018-11-13 18:43         ` Ferruh Yigit
2018-11-13 19:18           ` Zhang, Qi Z
2018-11-14 19:56 ` [dpdk-dev] [PATCH v3 0/2] fix pcap handlers for secondary Qi Zhang
2018-11-14 19:56   ` [dpdk-dev] [PATCH v3 1/2] net/pcap: move pcap handler to process private Qi Zhang
2018-11-14 23:05     ` Ferruh Yigit
2018-11-15  0:13       ` Zhang, Qi Z
2018-11-14 19:56   ` [dpdk-dev] [PATCH v3 2/2] net/pcap: enable data path for secondary Qi Zhang
2018-11-14 23:08     ` Ferruh Yigit
2018-11-15  0:06       ` Zhang, Qi Z
2018-11-15  1:37 ` [dpdk-dev] [PATCH v4 0/2] fix pcap handlers " Qi Zhang
2018-11-15  1:37   ` [dpdk-dev] [PATCH v4 1/2] net/pcap: move pcap handler to process private Qi Zhang
2018-11-16 15:56     ` Ferruh Yigit
2018-11-15  1:37   ` [dpdk-dev] [PATCH v4 2/2] net/pcap: enable data path for secondary Qi Zhang
2018-11-16 14:54   ` [dpdk-dev] [PATCH v4 0/2] fix pcap handlers " Ferruh Yigit
2018-11-16 16:12     ` Ferruh Yigit

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=861c43b2-a082-fdf8-61b9-2df1f4ed919c@intel.com \
    --to=ferruh.yigit@intel.com \
    --cc=dev@dpdk.org \
    --cc=qi.z.zhang@intel.com \
    --cc=thomas@monjalon.net \
    --cc=xueqin.lin@intel.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

DPDK patches and discussions

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://inbox.dpdk.org/dev/0 dev/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 dev dev/ https://inbox.dpdk.org/dev \
		dev@dpdk.org
	public-inbox-index dev

Example config snippet for mirrors.
Newsgroup available over NNTP:
	nntp://inbox.dpdk.org/inbox.dpdk.dev


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git