From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga04.intel.com (mga04.intel.com [192.55.52.120]) by dpdk.org (Postfix) with ESMTP id 591BA7CE2; Thu, 25 Oct 2018 17:18:24 +0200 (CEST) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga005.fm.intel.com ([10.253.24.32]) by fmsmga104.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 25 Oct 2018 08:18:23 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.54,424,1534834800"; d="scan'208";a="274402343" Received: from fmsmsx107.amr.corp.intel.com ([10.18.124.205]) by fmsmga005.fm.intel.com with ESMTP; 25 Oct 2018 08:18:23 -0700 Received: from shsmsx104.ccr.corp.intel.com (10.239.4.70) by fmsmsx107.amr.corp.intel.com (10.18.124.205) with Microsoft SMTP Server (TLS) id 14.3.319.2; Thu, 25 Oct 2018 08:18:23 -0700 Received: from shsmsx103.ccr.corp.intel.com ([169.254.4.161]) by SHSMSX104.ccr.corp.intel.com ([169.254.5.117]) with mapi id 14.03.0415.000; Thu, 25 Oct 2018 23:18:21 +0800 From: "Zhang, Qi Z" To: =?iso-8859-1?Q?Ga=EBtan_Rivet?= CC: "thomas@monjalon.net" , "dev@dpdk.org" , "stable@dpdk.org" Thread-Topic: [PATCH] bus/vdev: fix device argument corrupt after bus scan Thread-Index: AQHUbBL3TYeZulvOwUOzGQInCVlqWKUvMksAgADS0DD//4RWgIAAhzpw Date: Thu, 25 Oct 2018 15:18:20 +0000 Message-ID: <039ED4275CED7440929022BC67E70611532DB741@SHSMSX103.ccr.corp.intel.com> References: <20181025033036.23680-1-qi.z.zhang@intel.com> <20181025095118.gnc75zvkuvfajtha@bidouze.vm.6wind.com> <039ED4275CED7440929022BC67E70611532DB6EF@SHSMSX103.ccr.corp.intel.com> <20181025150313.g6d7tfhmsiz7wwim@bidouze.vm.6wind.com> In-Reply-To: <20181025150313.g6d7tfhmsiz7wwim@bidouze.vm.6wind.com> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiMTdhMmExODItNmFlZS00MDAwLTgxYmUtODlhZTgzZDMyZTEyIiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX05UIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE3LjEwLjE4MDQuNDkiLCJUcnVzdGVkTGFiZWxIYXNoIjoiam8zdkZqVnFRQnJjYmtXMDFMR2tQcStENGx1Z0ZERUtWRGRhUTZEXC9qQ1l1WDVPK2o5RjI1cE12WVFxelM1ZDgifQ== x-ctpclassification: CTP_NT dlp-product: dlpe-windows dlp-version: 11.0.400.15 dlp-reaction: no-action x-originating-ip: [10.239.127.40] Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Subject: Re: [dpdk-dev] [PATCH] bus/vdev: fix device argument corrupt after bus scan X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 25 Oct 2018 15:18:25 -0000 > -----Original Message----- > From: Ga=EBtan Rivet [mailto:gaetan.rivet@6wind.com] > Sent: Thursday, October 25, 2018 10:03 AM > To: Zhang, Qi Z > Cc: thomas@monjalon.net; dev@dpdk.org; stable@dpdk.org > Subject: Re: [PATCH] bus/vdev: fix device argument corrupt after bus scan >=20 > On Thu, Oct 25, 2018 at 02:56:55PM +0000, Zhang, Qi Z wrote: > > > > > > > -----Original Message----- > > > From: Ga=EBtan Rivet [mailto:gaetan.rivet@6wind.com] > > > Sent: Thursday, October 25, 2018 4:51 AM > > > To: Zhang, Qi Z > > > 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 > > > > --- > > > > 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 bet= ter 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 >=20 > 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 pr= obed, > 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= =20 So we are in the context of dev->scan, we should not do dev_probe in dev->s= can, since dev_probe will call dev->scan again, then it go dead loop. >=20 > My take on it is that it seems to be fixing both your issue and cleaning = up > history. >=20 > > > > > > > > > > > > > { > > > > 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 =3D 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 =3D insert_vdev(name, args, &dev); > > > > + ret =3D insert_vdev(name, args, &dev, true); > > > > if (ret =3D=3D 0) { > > > > ret =3D 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 =3D insert_vdev(in->name, NULL, NULL); > > > > + ret =3D insert_vdev(in->name, NULL, NULL, false); > > > > if (ret =3D=3D -EEXIST) > > > > VDEV_LOG(DEBUG, "device already exist, %s", in->name); > > > > else if (ret < 0) > > > > -- > > > > 2.13.6 > > > > > > > > > > -- > > > Ga=EBtan Rivet > > > 6WIND >=20 > -- > Ga=EBtan Rivet > 6WIND