DPDK patches and discussions
 help / color / mirror / Atom feed
From: "Zhang, Qi Z" <qi.z.zhang@intel.com>
To: "Gaëtan Rivet" <gaetan.rivet@6wind.com>
Cc: "thomas@monjalon.net" <thomas@monjalon.net>,
	"dev@dpdk.org" <dev@dpdk.org>,
	"stable@dpdk.org" <stable@dpdk.org>
Subject: Re: [dpdk-dev] [PATCH] bus/vdev: fix device argument corrupt after bus scan
Date: Thu, 25 Oct 2018 15:18:20 +0000	[thread overview]
Message-ID: <039ED4275CED7440929022BC67E70611532DB741@SHSMSX103.ccr.corp.intel.com> (raw)
In-Reply-To: <20181025150313.g6d7tfhmsiz7wwim@bidouze.vm.6wind.com>



> -----Original Message-----
> From: Gaëtan Rivet [mailto:gaetan.rivet@6wind.com]
> Sent: Thursday, October 25, 2018 10:03 AM
> To: Zhang, Qi Z <qi.z.zhang@intel.com>
> Cc: thomas@monjalon.net; dev@dpdk.org; stable@dpdk.org
> Subject: Re: [PATCH] bus/vdev: fix device argument corrupt after bus scan
> 
> On Thu, Oct 25, 2018 at 02:56:55PM +0000, Zhang, Qi Z wrote:
> >
> >
> > > -----Original Message-----
> > > From: Gaëtan Rivet [mailto:gaetan.rivet@6wind.com]
> > > Sent: Thursday, October 25, 2018 4:51 AM
> > > To: Zhang, Qi Z <qi.z.zhang@intel.com>
> > > Cc: thomas@monjalon.net; dev@dpdk.org; stable@dpdk.org
> > > Subject: Re: [PATCH] bus/vdev: fix device argument corrupt after bus
> > > scan
> > >
> > > On Thu, Oct 25, 2018 at 11:30:36AM +0800, Qi Zhang wrote:
> > > > It's not necessary to insert device argment to devargs_list during
> > > > bus scan, but this happens when we try to attach a device on
> > > > secondary process. The patch fix the issue.
> > > >
> > > > Fixes: cdb068f031c6 ("bus/vdev: scan by multi-process channel")
> > > > Cc: stable@dpdk.org
> > > >
> > > > Signed-off-by: Qi Zhang <qi.z.zhang@intel.com>
> > > > ---
> > > >  drivers/bus/vdev/vdev.c | 11 +++++++----
> > > >  1 file changed, 7 insertions(+), 4 deletions(-)
> > > >
> > > > diff --git a/drivers/bus/vdev/vdev.c b/drivers/bus/vdev/vdev.c
> > > > index
> > > > 688e31c21..818a2bfc2 100644
> > > > --- a/drivers/bus/vdev/vdev.c
> > > > +++ b/drivers/bus/vdev/vdev.c
> > > > @@ -202,7 +202,9 @@ alloc_devargs(const char *name, const char
> > > > *args) }
> > > >
> > > >  static int
> > > > -insert_vdev(const char *name, const char *args, struct
> > > > rte_vdev_device **p_dev)
> > > > +insert_vdev(const char *name, const char *args,
> > > > +		struct rte_vdev_device **p_dev,
> > > > +		bool init)
> > >
> > > Why is vdev the only bus not using hotplug API itself and
> > > reimplementing it on its own?
> >
> > I don't know the history,
> > but replace insert_vdev with hotplug API will not solve all the
> > problem (see my below comments)
> >
> > >
> > > It should not have to insert devargs at all, not even in the primary
> > > process. If it called rte_dev_probe(), this would normally already
> > > be properly handled I think.
> >
> > Currently insert_vdev is shared by two scenarios 1. rte_vdev_init,
> > which is called by application to attach a new device, I agree it's better to
> have rte_dev_probe here so, no need to have insert_vdev here.
> > 2. during secondary's vdev->scan when it receive a SCAN_ONE event from
> > primary, it should not call rte_devargs_insert, The patch is going to
> > fix the issue on secondary scenario and we can do the cleanup for
> > first issue in a separate patch
> 
> In 2., won't dev_probe() check for secondary process context and in this case,
> send the request to primary, which will see that the device is already probed,
> which would thus fix your issue?

No, It's not the case that we issue a device attaching from secondary, but the case that we try to sync with primary for a device already be attached 
So we are in the context of dev->scan, we should not do dev_probe in dev->scan, since dev_probe will call dev->scan again, then it go dead loop.
> 
> My take on it is that it seems to be fixing both your issue and cleaning up
> history.
> 
> >
> >
> >
> > >
> > > >  {
> > > >  	struct rte_vdev_device *dev;
> > > >  	struct rte_devargs *devargs;
> > > > @@ -237,7 +239,8 @@ insert_vdev(const char *name, const char
> > > > *args,
> > > struct rte_vdev_device **p_dev)
> > > >  	}
> > > >
> > > >  	TAILQ_INSERT_TAIL(&vdev_device_list, dev, next);
> > > > -	rte_devargs_insert(devargs);
> > > > +	if (init)
> > > > +		rte_devargs_insert(devargs);
> > > >
> > > >  	if (p_dev)
> > > >  		*p_dev = dev;
> > > > @@ -257,7 +260,7 @@ rte_vdev_init(const char *name, const char
> *args)
> > > >  	int ret;
> > > >
> > > >  	rte_spinlock_recursive_lock(&vdev_device_list_lock);
> > > > -	ret = insert_vdev(name, args, &dev);
> > > > +	ret = insert_vdev(name, args, &dev, true);
> > > >  	if (ret == 0) {
> > > >  		ret = vdev_probe_all_drivers(dev);
> > > >  		if (ret) {
> > > > @@ -383,7 +386,7 @@ vdev_action(const struct rte_mp_msg
> *mp_msg,
> > > const void *peer)
> > > >  		break;
> > > >  	case VDEV_SCAN_ONE:
> > > >  		VDEV_LOG(INFO, "receive vdev, %s", in->name);
> > > > -		ret = insert_vdev(in->name, NULL, NULL);
> > > > +		ret = insert_vdev(in->name, NULL, NULL, false);
> > > >  		if (ret == -EEXIST)
> > > >  			VDEV_LOG(DEBUG, "device already exist, %s", in->name);
> > > >  		else if (ret < 0)
> > > > --
> > > > 2.13.6
> > > >
> > >
> > > --
> > > Gaëtan Rivet
> > > 6WIND
> 
> --
> Gaëtan Rivet
> 6WIND

  reply	other threads:[~2018-10-25 15:18 UTC|newest]

Thread overview: 7+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-10-25  3:30 Qi Zhang
2018-10-25  9:51 ` Gaëtan Rivet
2018-10-25 14:56   ` Zhang, Qi Z
2018-10-25 15:03     ` Gaëtan Rivet
2018-10-25 15:18       ` Zhang, Qi Z [this message]
2018-10-25 15:26         ` Gaëtan Rivet
2018-10-28 17:32 ` Thomas Monjalon

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=039ED4275CED7440929022BC67E70611532DB741@SHSMSX103.ccr.corp.intel.com \
    --to=qi.z.zhang@intel.com \
    --cc=dev@dpdk.org \
    --cc=gaetan.rivet@6wind.com \
    --cc=stable@dpdk.org \
    --cc=thomas@monjalon.net \
    /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
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).