| Message ID | fb738c0b251da31747451906630cfd09a5135c7b.1527618402.git.swise@opengridcomputing.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
>-----Original Message----- >From: linux-rdma-owner@vger.kernel.org [mailto:linux-rdma- >owner@vger.kernel.org] On Behalf Of Steve Wise >Sent: Tuesday, May 29, 2018 2:26 PM >To: axboe@kernel.dk; hch@lst.de; Busch, Keith <keith.busch@intel.com>; >sagi@grimberg.me; linux-nvme@lists.infradead.org >Cc: parav@mellanox.com; maxg@mellanox.com; linux-rdma@vger.kernel.org >Subject: [PATCH v3 1/3] nvme-rdma: correctly check for target keyed sgl >support > >The code was checking bit 20 instead of bit 2. Also fixed >the log entry. > >Signed-off-by: Steve Wise <swise@opengridcomputing.com> >--- > drivers/nvme/host/rdma.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > >diff --git a/drivers/nvme/host/rdma.c b/drivers/nvme/host/rdma.c >index 1eb4438..f11faa8 100644 >--- a/drivers/nvme/host/rdma.c >+++ b/drivers/nvme/host/rdma.c >@@ -1949,8 +1949,8 @@ static struct nvme_ctrl >*nvme_rdma_create_ctrl(struct device *dev, > } > > /* sanity check keyed sgls */ >- if (!(ctrl->ctrl.sgls & (1 << 20))) { >- dev_err(ctrl->ctrl.device, "Mandatory keyed sgls are not >support\n"); >+ if (!(ctrl->ctrl.sgls & (1 << 2))) { >+ dev_err(ctrl->ctrl.device, "Mandatory keyed sgls are not >supported!\n"); I can see that the 2 and 20 are defined for specific things. Since they are used in several places (in the next 2 patches), is there any chance these could be defined bits? Mike > ret = -EINVAL; > goto out_remove_admin_queue; > } >-- >1.8.3.1 > >-- >To unsubscribe from this list: send the line "unsubscribe linux-rdma" in >the body of a message to majordomo@vger.kernel.org >More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 5/29/2018 3:23 PM, Ruhl, Michael J wrote: >> -----Original Message----- >> From: linux-rdma-owner@vger.kernel.org [mailto:linux-rdma- >> owner@vger.kernel.org] On Behalf Of Steve Wise >> Sent: Tuesday, May 29, 2018 2:26 PM >> To: axboe@kernel.dk; hch@lst.de; Busch, Keith <keith.busch@intel.com>; >> sagi@grimberg.me; linux-nvme@lists.infradead.org >> Cc: parav@mellanox.com; maxg@mellanox.com; linux-rdma@vger.kernel.org >> Subject: [PATCH v3 1/3] nvme-rdma: correctly check for target keyed sgl >> support >> >> The code was checking bit 20 instead of bit 2. Also fixed >> the log entry. >> >> Signed-off-by: Steve Wise <swise@opengridcomputing.com> >> --- >> drivers/nvme/host/rdma.c | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/nvme/host/rdma.c b/drivers/nvme/host/rdma.c >> index 1eb4438..f11faa8 100644 >> --- a/drivers/nvme/host/rdma.c >> +++ b/drivers/nvme/host/rdma.c >> @@ -1949,8 +1949,8 @@ static struct nvme_ctrl >> *nvme_rdma_create_ctrl(struct device *dev, >> } >> >> /* sanity check keyed sgls */ >> - if (!(ctrl->ctrl.sgls & (1 << 20))) { >> - dev_err(ctrl->ctrl.device, "Mandatory keyed sgls are not >> support\n"); >> + if (!(ctrl->ctrl.sgls & (1 << 2))) { >> + dev_err(ctrl->ctrl.device, "Mandatory keyed sgls are not >> supported!\n"); > I can see that the 2 and 20 are defined for specific things. Since they are > used in several places (in the next 2 patches), is there any chance these > could be defined bits? > > Mike > That seems reasonable. I would think these defines could also be shared across the host and target. Perhaps include/linux/nvme.h? I see both nvme/host/nvme.h and nvme/target/nvmet.h include linux/nvme.h. So they could go there. Christoph, what do you think? It seems like these are either nvme protocol bits or nvme/f protocol bits... Steve. -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 5/30/2018 9:39 AM, Steve Wise wrote: > > On 5/29/2018 3:23 PM, Ruhl, Michael J wrote: >>> -----Original Message----- >>> From: linux-rdma-owner@vger.kernel.org [mailto:linux-rdma- >>> owner@vger.kernel.org] On Behalf Of Steve Wise >>> Sent: Tuesday, May 29, 2018 2:26 PM >>> To: axboe@kernel.dk; hch@lst.de; Busch, Keith <keith.busch@intel.com>; >>> sagi@grimberg.me; linux-nvme@lists.infradead.org >>> Cc: parav@mellanox.com; maxg@mellanox.com; linux-rdma@vger.kernel.org >>> Subject: [PATCH v3 1/3] nvme-rdma: correctly check for target keyed sgl >>> support >>> >>> The code was checking bit 20 instead of bit 2. Also fixed >>> the log entry. >>> >>> Signed-off-by: Steve Wise <swise@opengridcomputing.com> >>> --- >>> drivers/nvme/host/rdma.c | 4 ++-- >>> 1 file changed, 2 insertions(+), 2 deletions(-) >>> >>> diff --git a/drivers/nvme/host/rdma.c b/drivers/nvme/host/rdma.c >>> index 1eb4438..f11faa8 100644 >>> --- a/drivers/nvme/host/rdma.c >>> +++ b/drivers/nvme/host/rdma.c >>> @@ -1949,8 +1949,8 @@ static struct nvme_ctrl >>> *nvme_rdma_create_ctrl(struct device *dev, >>> } >>> >>> /* sanity check keyed sgls */ >>> - if (!(ctrl->ctrl.sgls & (1 << 20))) { >>> - dev_err(ctrl->ctrl.device, "Mandatory keyed sgls are not >>> support\n"); >>> + if (!(ctrl->ctrl.sgls & (1 << 2))) { >>> + dev_err(ctrl->ctrl.device, "Mandatory keyed sgls are not >>> supported!\n"); >> I can see that the 2 and 20 are defined for specific things. Since they are >> used in several places (in the next 2 patches), is there any chance these >> could be defined bits? >> >> Mike >> > That seems reasonable. I would think these defines could also be shared > across the host and target. Perhaps include/linux/nvme.h? I see both > nvme/host/nvme.h and nvme/target/nvmet.h include linux/nvme.h. So they > could go there. > > Christoph, what do you think? It seems like these are either nvme > protocol bits or nvme/f protocol bits... > > Steve. > > It looks like these bits are part of the Identify Controller Data Structure, figure 90 in the 1.2.1 spec. The SGL Support (SGLS) field within that struct. So I guess it belongs in include/linux/nvme.h? But I don't see any of this big structure in that header. Steve. -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
>>> I can see that the 2 and 20 are defined for specific things. Since they are >>> used in several places (in the next 2 patches), is there any chance these >>> could be defined bits? >>> >>> Mike >>> >> That seems reasonable. I would think these defines could also be shared >> across the host and target. Perhaps include/linux/nvme.h? I see both >> nvme/host/nvme.h and nvme/target/nvmet.h include linux/nvme.h. So they >> could go there. >> >> Christoph, what do you think? It seems like these are either nvme >> protocol bits or nvme/f protocol bits... >> >> Steve. >> >> > > It looks like these bits are part of the Identify Controller Data > Structure, figure 90 in the 1.2.1 spec. The SGL Support (SGLS) field > within that struct. So I guess it belongs in include/linux/nvme.h? > But I don't see any of this big structure in that header. > Yea, good idea, though please add the offset type sgl as well. -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, May 29, 2018 at 08:23:09PM +0000, Ruhl, Michael J wrote: > I can see that the 2 and 20 are defined for specific things. Since they are > used in several places (in the next 2 patches), is there any chance these > could be defined bits? So far our rule of thumb was to use defines where the NVMe spec defines symbolic names, and use plain bit numbers where the spec does the same. The SGLS field falls into the latter category. If we end up with a lot of users a little helper function might be nice, but I don't think we aren't there yet. -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, May 31, 2018 at 12:37:33AM +0300, Sagi Grimberg wrote: >> It looks like these bits are part of the Identify Controller Data >> Structure, figure 90 in the 1.2.1 spec. The SGL Support (SGLS) field >> within that struct. So I guess it belongs in include/linux/nvme.h? >> But I don't see any of this big structure in that header. >> > > Yea, good idea, though please add the offset type sgl as well. Honestly, I'd rather keep defines for every little bit without a name out of the headers. The bit numbers will show up right there when looking for the field in the nvme spec, while making up our own names will just create confusion. -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 5/31/2018 12:02 PM, hch@lst.de wrote: > On Thu, May 31, 2018 at 12:37:33AM +0300, Sagi Grimberg wrote: >>> It looks like these bits are part of the Identify Controller Data >>> Structure, figure 90 in the 1.2.1 spec. The SGL Support (SGLS) field >>> within that struct. So I guess it belongs in include/linux/nvme.h? >>> But I don't see any of this big structure in that header. >>> >> Yea, good idea, though please add the offset type sgl as well. > Honestly, I'd rather keep defines for every little bit without > a name out of the headers. The bit numbers will show up right > there when looking for the field in the nvme spec, while making > up our own names will just create confusion. > Ok then, I will leave them as bit numbers... -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, May 31, 2018 at 12:17:07PM -0500, Steve Wise wrote:
> Ok then, I will leave them as bit numbers...
Btw, modulo the debug printk 1 & 2 look fine to me and I'd like to
pull them in. Sagi, are you ok with the patches?
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
> -----Original Message----- > From: Linux-nvme <linux-nvme-bounces@lists.infradead.org> On Behalf Of > hch@lst.de > Sent: Thursday, May 31, 2018 12:26 PM > To: Steve Wise <swise@opengridcomputing.com> > Cc: axboe@kernel.dk; Sagi Grimberg <sagi@grimberg.me>; linux- > rdma@vger.kernel.org; linux-nvme@lists.infradead.org; Busch, Keith > <keith.busch@intel.com>; Ruhl, Michael J <michael.j.ruhl@intel.com>; > maxg@mellanox.com; hch@lst.de; parav@mellanox.com > Subject: Re: [PATCH v3 1/3] nvme-rdma: correctly check for target keyed sgl > support > > On Thu, May 31, 2018 at 12:17:07PM -0500, Steve Wise wrote: > > Ok then, I will leave them as bit numbers... > > Btw, modulo the debug printk 1 & 2 look fine to me and I'd like to > pull them in. Sagi, are you ok with the patches? > If you do, I'll drop them from my series and repost the target side soon with the changes to only allocate single pages. Thanks Steve -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
>> Ok then, I will leave them as bit numbers... > > Btw, modulo the debug printk 1 & 2 look fine to me and I'd like to > pull them in. Sagi, are you ok with the patches? I'd like to see a version that avoids higher order allocations first as we can have a non negligible number of these allocations. If it turns out too much of hassle we can stay with this. -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> -----Original Message----- > From: Sagi Grimberg <sagi@grimberg.me> > Sent: Sunday, June 3, 2018 6:57 AM > To: hch@lst.de; Steve Wise <swise@opengridcomputing.com> > Cc: Ruhl, Michael J <michael.j.ruhl@intel.com>; axboe@kernel.dk; Busch, > Keith <keith.busch@intel.com>; linux-nvme@lists.infradead.org; > parav@mellanox.com; maxg@mellanox.com; linux-rdma@vger.kernel.org > Subject: Re: [PATCH v3 1/3] nvme-rdma: correctly check for target keyed sgl > support > > > >> Ok then, I will leave them as bit numbers... > > > > Btw, modulo the debug printk 1 & 2 look fine to me and I'd like to > > pull them in. Sagi, are you ok with the patches? > > I'd like to see a version that avoids higher order allocations first > as we can have a non negligible number of these allocations. > If it turns out too much of hassle we can stay with this. He's referring to patch 1 and 2, which are the host side. No page allocations. Also, I made the changes to avoid higher order allocations for patch 3. Testing now. I'll send it out tomorrow. Steve. -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> He's referring to patch 1 and 2, which are the host side. No page allocations. I'm good with 1 & 2, Christoph, you can add my Reviewed-by: Sagi Grimberg <sagi@grimberg.me> -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Jun 04, 2018 at 03:01:43PM +0300, Sagi Grimberg wrote: > >> He's referring to patch 1 and 2, which are the host side. No page allocations. > > I'm good with 1 & 2, > > Christoph, you can add my > > Reviewed-by: Sagi Grimberg <sagi@grimberg.me> We've missed the merge window now, so we can just wait for a proper resend from Steve I think. -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> On Mon, Jun 04, 2018 at 03:01:43PM +0300, Sagi Grimberg wrote: > > > >> He's referring to patch 1 and 2, which are the host side. No page > allocations. > > > > I'm good with 1 & 2, > > > > Christoph, you can add my > > > > Reviewed-by: Sagi Grimberg <sagi@grimberg.me> > > We've missed the merge window now, so we can just wait for a proper > resend from Steve I think. Ok I'll be sending a new version soon. Steve. -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 6/4/2018 3:11 PM, Christoph Hellwig wrote: > On Mon, Jun 04, 2018 at 03:01:43PM +0300, Sagi Grimberg wrote: >> >>> He's referring to patch 1 and 2, which are the host side. No page allocations. >> >> I'm good with 1 & 2, >> >> Christoph, you can add my >> >> Reviewed-by: Sagi Grimberg <sagi@grimberg.me> > > We've missed the merge window now, so we can just wait for a proper > resend from Steve I think. > There are still issue that I'm trying to help Steve with their debug so let's wait with the merge until we figure them out. -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> -----Original Message----- > From: Max Gurtovoy <maxg@mellanox.com> > Sent: Monday, June 4, 2018 8:52 AM > To: Christoph Hellwig <hch@lst.de>; Sagi Grimberg <sagi@grimberg.me> > Cc: Steve Wise <swise@opengridcomputing.com>; 'Ruhl, Michael J' > <michael.j.ruhl@intel.com>; axboe@kernel.dk; 'Busch, Keith' > <keith.busch@intel.com>; linux-nvme@lists.infradead.org; > parav@mellanox.com; linux-rdma@vger.kernel.org > Subject: Re: [PATCH v3 1/3] nvme-rdma: correctly check for target keyed sgl > support > > > > On 6/4/2018 3:11 PM, Christoph Hellwig wrote: > > On Mon, Jun 04, 2018 at 03:01:43PM +0300, Sagi Grimberg wrote: > >> > >>> He's referring to patch 1 and 2, which are the host side. No page > allocations. > >> > >> I'm good with 1 & 2, > >> > >> Christoph, you can add my > >> > >> Reviewed-by: Sagi Grimberg <sagi@grimberg.me> > > > > We've missed the merge window now, so we can just wait for a proper > > resend from Steve I think. > > > > There are still issue that I'm trying to help Steve with their debug so > let's wait with the merge until we figure them out. I would like review on my new nvmet-rdma changes to avoid > 0 order page allocations though. Perhaps I'll resend the series and add the RFC tag (or WIP?) with verbiage saying don't merge yet. -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/nvme/host/rdma.c b/drivers/nvme/host/rdma.c index 1eb4438..f11faa8 100644 --- a/drivers/nvme/host/rdma.c +++ b/drivers/nvme/host/rdma.c @@ -1949,8 +1949,8 @@ static struct nvme_ctrl *nvme_rdma_create_ctrl(struct device *dev, } /* sanity check keyed sgls */ - if (!(ctrl->ctrl.sgls & (1 << 20))) { - dev_err(ctrl->ctrl.device, "Mandatory keyed sgls are not support\n"); + if (!(ctrl->ctrl.sgls & (1 << 2))) { + dev_err(ctrl->ctrl.device, "Mandatory keyed sgls are not supported!\n"); ret = -EINVAL; goto out_remove_admin_queue; }
The code was checking bit 20 instead of bit 2. Also fixed the log entry. Signed-off-by: Steve Wise <swise@opengridcomputing.com> --- drivers/nvme/host/rdma.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)