From mboxrd@z Thu Jan 1 00:00:00 1970 Content-Type: multipart/mixed; boundary="===============1355357590119654435==" MIME-Version: 1.0 From: Andrey Kuzmin Subject: Re: [SPDK] Allow attachment to an already probed controller Date: Sat, 27 Feb 2016 12:04:37 +0300 Message-ID: In-Reply-To: 1456525252.80454.153.camel@intel.com List-ID: To: spdk@lists.01.org --===============1355357590119654435== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable On Sat, Feb 27, 2016 at 1:20 AM, Walker, Benjamin wrote: > On Fri, 2016-02-26 at 21:18 +0300, Andrey Kuzmin wrote: >> On Fri, Feb 26, 2016 at 9:08 PM, Verkamp, Daniel >> wrote: >> > Hi Andrey, >> > >> > I am not sure what behavior you would like to see from probe when >> > calling it again after attaching some controllers. We can't re- >> > probe/re-attach the same controller twice without detaching it in >> > between - if it is already attached, that means the driver is >> > already running, queues are initialized, etc. and we should not >> > call the user's attach callback again. >> > >> > The spdk_nvme_probe() interface operates at the controller (PCIe >> > device) level; namespace enumeration should be handled in the >> > application layer in or after the attach callback. >> > >> > Can you clarify your desired behavior/use case in your app? >> > >> >> To my understanding, the goal of the probe(), from the user's >> standpoint, is to get hold of the controller reference (and it's >> basically the only way to accomplish this under the current API). If >> I'm parsing multiple PCI inputs, the probe() semantics forces me into >> the probed controller housekeeping on my side. As probe() (one can >> safely assume) occurs in the initialization phase, it should be safe >> to return the controller references back to the user even if the >> controller has already been started, at user's discretion. >> >> > We can possibly add another interface to enumerate all of the NVMe >> > controllers that are currently attached to the userspace driver >> > (the ones in the attached_ctrlrs list) if that would help. >> > >> >> That's pretty much what I have suggested. > > If we add an API to give you a list of all of the currently attached > controllers, does that solve the problem? Are there any other issues > beyond that one? That would relieve the caller from the attached controller list maintenance, but will still require a lookup in the application. A more intuitive way, IMO, would be to add an extra parameter to probe(), e.g. 'bool skip_already_attached'. Set to true, it will preserve current probe() semantics. If the user sets skip_attached to false, probe() will start with scanning the attached controller TAILQ, applying the provided probe_cb()/attached_cb() as in the patch provided. Regards, Andrey > >> >> Regards, >> Andrey >> >> > Thanks, >> > -- Daniel >> > >> > P.S. I believe the return value from PCI enumeration is already >> > handled; the intent is that even if nvme_pci_enumerate() returns a >> > failure code, it may have found some controllers, and we want to >> > initialize those even if other PCI functions failed to probe. The >> > return code is returned intact at the end of spdk_nvme_probe() so >> > that the application can determine that an error occurred and take >> > action if necessary. >> > >> > -----Original Message----- >> > From: SPDK [mailto:spdk-bounces(a)lists.01.org] On Behalf Of Andrey >> > Kuzmin >> > Sent: Friday, February 26, 2016 10:52 AM >> > To: spdk(a)lists.01.org >> > Subject: [SPDK] Allow attachment to an already probed controller >> > >> > While I appreciatethe reasoning behind the current spdk_nvme_probe >> > attach-once semantics, it still imposes an extra burden of the >> > already probed controller tracking on the application that, for >> > instance, seeks to attach to multiple namespaces under the same >> > controller. The below patch provides an optional fix for the issue. >> > Notice that it also fixes the bug of the error returned by the >> > enumeration being ignored. >> > >> > Regards, >> > Andrey >> > >> > diff --git a/lib/nvme/nvme.c b/lib/nvme/nvme.c index >> > 63a316b..2a3ac64 100644 >> > --- a/lib/nvme/nvme.c >> > +++ b/lib/nvme/nvme.c >> > @@ -296,10 +296,32 @@ spdk_nvme_probe(void *cb_ctx, >> > spdk_nvme_probe_cb probe_cb, spdk_nvme_attach_cb a >> > >> > nvme_mutex_lock(&g_nvme_driver.lock); >> > >> > + /* >> > + * First, check if the requested controller has already been >> > started. >> > + */ >> > + TAILQ_FOREACH(ctrlr, &g_nvme_driver.attached_ctrlrs, tailq) { >> > + if (!probe_cb(cb_ctx, ctrlr->devhandle)) >> > + continue; >> > + >> > + nvme_mutex_unlock(&g_nvme_driver.lock); >> > + attach_cb(cb_ctx, ctrlr->devhandle, ctrlr); >> > + return 0; >> > + } >> > + >> > enum_ctx.probe_cb =3D probe_cb; >> > enum_ctx.cb_ctx =3D cb_ctx; >> > >> > rc =3D nvme_pci_enumerate(nvme_enum_cb, &enum_ctx); >> > + >> > + /* >> > + * Appreciate the error being returned >> > + * by the nvme_pci_enumerate, if any. >> > + */ >> > + if (rc) { >> > + nvme_mutex_unlock(&g_nvme_driver.lock); >> > + return rc; >> > + } >> > + >> > /* >> > * Keep going even if one or more nvme_attach() calls failed, >> > * but maintain the value of rc to signal errors when we >> > return. >> > _______________________________________________ >> > SPDK mailing list >> > SPDK(a)lists.01.org >> > https://lists.01.org/mailman/listinfo/spdk >> _______________________________________________ >> SPDK mailing list >> SPDK(a)lists.01.org >> https://lists.01.org/mailman/listinfo/spdk --===============1355357590119654435==--