From: Tianxianting <tian.xianting@h3c.com> To: Keith Busch <kbusch@kernel.org> Cc: "axboe@fb.com" <axboe@fb.com>, "hch@lst.de" <hch@lst.de>, "sagi@grimberg.me" <sagi@grimberg.me>, "linux-nvme@lists.infradead.org" <linux-nvme@lists.infradead.org>, "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org> Subject: RE: [PATCH] nvme: replace meaningless judgement by checking whether req is null Date: Mon, 21 Sep 2020 16:08:56 +0000 [thread overview] Message-ID: <0efc3ee511534cf789073ed5ddfa61a2@h3c.com> (raw) In-Reply-To: <20200921155925.GA4034241@dhcp-10-100-145-180.wdl.wdc.com> I get it now, thanks Keith:) -----Original Message----- From: Keith Busch [mailto:kbusch@kernel.org] Sent: Monday, September 21, 2020 11:59 PM To: tianxianting (RD) <tian.xianting@h3c.com> Cc: axboe@fb.com; hch@lst.de; sagi@grimberg.me; linux-nvme@lists.infradead.org; linux-kernel@vger.kernel.org Subject: Re: [PATCH] nvme: replace meaningless judgement by checking whether req is null On Mon, Sep 21, 2020 at 03:49:09PM +0000, Tianxianting wrote: > HI Keith, > Thanks for your comments, > I will submit a new patch of version 2 for the further reviewing, v2 patch will contains: > 1, retain existing judgement and dev_warn; No no, go ahead and remove the existing check just as you've done. That check becomes redundant with the safer one you're adding, and we don't want redundant checks in the fast path. My only suggestion is to use the same dev_warn() in your new check. > 2, add the check whether req is null(already did in this patch) 3, > simplify and make the changelog succinct according to you said " This is what I'm thinking:". > Is it right? > Should I retain the nvme_irq crash log in changelog, mention the difference between nvmeq->q_depth and tagset queue_depth? The tagset's queue_depth is a valid point to mention as well. The dirver's current indirect check is not necessarily in sync with the actual tagset. > Thanks > > -----Original Message----- > From: Keith Busch [mailto:kbusch@kernel.org] > Sent: Monday, September 21, 2020 11:08 PM > To: tianxianting (RD) <tian.xianting@h3c.com> > Cc: axboe@fb.com; hch@lst.de; sagi@grimberg.me; > linux-nvme@lists.infradead.org; linux-kernel@vger.kernel.org > Subject: Re: [PATCH] nvme: replace meaningless judgement by checking > whether req is null > > On Mon, Sep 21, 2020 at 10:10:52AM +0800, Xianting Tian wrote: > > @@ -940,13 +940,6 @@ static inline void nvme_handle_cqe(struct nvme_queue *nvmeq, u16 idx) > > struct nvme_completion *cqe = &nvmeq->cqes[idx]; > > struct request *req; > > > > - if (unlikely(cqe->command_id >= nvmeq->q_depth)) { > > - dev_warn(nvmeq->dev->ctrl.device, > > - "invalid id %d completed on queue %d\n", > > - cqe->command_id, le16_to_cpu(cqe->sq_id)); > > - return; > > - } > > - > > /* > > * AEN requests are special as they don't time out and can > > * survive any kind of queue freeze and often don't respond to @@ > > -960,6 +953,13 @@ static inline void nvme_handle_cqe(struct nvme_queue *nvmeq, u16 idx) > > } > > > > req = blk_mq_tag_to_rq(nvme_queue_tagset(nvmeq), cqe->command_id); > > + if (unlikely(!req)) { > > + dev_warn(nvmeq->dev->ctrl.device, > > + "req is null for tag %d completed on queue %d\n", > > + cqe->command_id, le16_to_cpu(cqe->sq_id)); > > + return; > > + } > > This is making sense now, though I think we should retain the existing > dev_warn() since it's still accurate and provides continuity for people who are used to looking for these sorts of messages. > > Your changelog is a bit much though. I think we can say it a bit more succinctly. This is what I'm thinking: > > The driver registers interrupts for queues before initializing the > tagset because it uses the number of successful request_irq() calls > to configure the tagset parameters. This allows a race condition with > the current tag validity check if the controller happens to produce > an interrupt with a corrupted CQE before the tagset is initialized. > > Replace the driver's indirect tag check with the one already provided > by the block layer.
WARNING: multiple messages have this Message-ID (diff)
From: Tianxianting <tian.xianting@h3c.com> To: Keith Busch <kbusch@kernel.org> Cc: "axboe@fb.com" <axboe@fb.com>, "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>, "hch@lst.de" <hch@lst.de>, "linux-nvme@lists.infradead.org" <linux-nvme@lists.infradead.org>, "sagi@grimberg.me" <sagi@grimberg.me> Subject: RE: [PATCH] nvme: replace meaningless judgement by checking whether req is null Date: Mon, 21 Sep 2020 16:08:56 +0000 [thread overview] Message-ID: <0efc3ee511534cf789073ed5ddfa61a2@h3c.com> (raw) In-Reply-To: <20200921155925.GA4034241@dhcp-10-100-145-180.wdl.wdc.com> I get it now, thanks Keith:) -----Original Message----- From: Keith Busch [mailto:kbusch@kernel.org] Sent: Monday, September 21, 2020 11:59 PM To: tianxianting (RD) <tian.xianting@h3c.com> Cc: axboe@fb.com; hch@lst.de; sagi@grimberg.me; linux-nvme@lists.infradead.org; linux-kernel@vger.kernel.org Subject: Re: [PATCH] nvme: replace meaningless judgement by checking whether req is null On Mon, Sep 21, 2020 at 03:49:09PM +0000, Tianxianting wrote: > HI Keith, > Thanks for your comments, > I will submit a new patch of version 2 for the further reviewing, v2 patch will contains: > 1, retain existing judgement and dev_warn; No no, go ahead and remove the existing check just as you've done. That check becomes redundant with the safer one you're adding, and we don't want redundant checks in the fast path. My only suggestion is to use the same dev_warn() in your new check. > 2, add the check whether req is null(already did in this patch) 3, > simplify and make the changelog succinct according to you said " This is what I'm thinking:". > Is it right? > Should I retain the nvme_irq crash log in changelog, mention the difference between nvmeq->q_depth and tagset queue_depth? The tagset's queue_depth is a valid point to mention as well. The dirver's current indirect check is not necessarily in sync with the actual tagset. > Thanks > > -----Original Message----- > From: Keith Busch [mailto:kbusch@kernel.org] > Sent: Monday, September 21, 2020 11:08 PM > To: tianxianting (RD) <tian.xianting@h3c.com> > Cc: axboe@fb.com; hch@lst.de; sagi@grimberg.me; > linux-nvme@lists.infradead.org; linux-kernel@vger.kernel.org > Subject: Re: [PATCH] nvme: replace meaningless judgement by checking > whether req is null > > On Mon, Sep 21, 2020 at 10:10:52AM +0800, Xianting Tian wrote: > > @@ -940,13 +940,6 @@ static inline void nvme_handle_cqe(struct nvme_queue *nvmeq, u16 idx) > > struct nvme_completion *cqe = &nvmeq->cqes[idx]; > > struct request *req; > > > > - if (unlikely(cqe->command_id >= nvmeq->q_depth)) { > > - dev_warn(nvmeq->dev->ctrl.device, > > - "invalid id %d completed on queue %d\n", > > - cqe->command_id, le16_to_cpu(cqe->sq_id)); > > - return; > > - } > > - > > /* > > * AEN requests are special as they don't time out and can > > * survive any kind of queue freeze and often don't respond to @@ > > -960,6 +953,13 @@ static inline void nvme_handle_cqe(struct nvme_queue *nvmeq, u16 idx) > > } > > > > req = blk_mq_tag_to_rq(nvme_queue_tagset(nvmeq), cqe->command_id); > > + if (unlikely(!req)) { > > + dev_warn(nvmeq->dev->ctrl.device, > > + "req is null for tag %d completed on queue %d\n", > > + cqe->command_id, le16_to_cpu(cqe->sq_id)); > > + return; > > + } > > This is making sense now, though I think we should retain the existing > dev_warn() since it's still accurate and provides continuity for people who are used to looking for these sorts of messages. > > Your changelog is a bit much though. I think we can say it a bit more succinctly. This is what I'm thinking: > > The driver registers interrupts for queues before initializing the > tagset because it uses the number of successful request_irq() calls > to configure the tagset parameters. This allows a race condition with > the current tag validity check if the controller happens to produce > an interrupt with a corrupted CQE before the tagset is initialized. > > Replace the driver's indirect tag check with the one already provided > by the block layer. _______________________________________________ Linux-nvme mailing list Linux-nvme@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-nvme
next prev parent reply other threads:[~2020-09-21 17:40 UTC|newest] Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top 2020-09-21 2:10 [PATCH] nvme: replace meaningless judgement by checking whether req is null Xianting Tian 2020-09-21 2:10 ` Xianting Tian 2020-09-21 6:34 ` Christoph Hellwig 2020-09-21 6:34 ` Christoph Hellwig 2020-09-21 15:08 ` Keith Busch 2020-09-21 15:08 ` Keith Busch 2020-09-21 15:49 ` Tianxianting 2020-09-21 15:49 ` Tianxianting 2020-09-21 15:59 ` Keith Busch 2020-09-21 15:59 ` Keith Busch 2020-09-21 16:08 ` Tianxianting [this message] 2020-09-21 16:08 ` Tianxianting
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=0efc3ee511534cf789073ed5ddfa61a2@h3c.com \ --to=tian.xianting@h3c.com \ --cc=axboe@fb.com \ --cc=hch@lst.de \ --cc=kbusch@kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-nvme@lists.infradead.org \ --cc=sagi@grimberg.me \ /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: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.