Re: [PATCH 3/7] nvme: implement mq_ops->commit_rqs() hook

[Jens Axboe]

On 11/29/18 10:38 AM, Keith Busch wrote:
> On Thu, Nov 29, 2018 at 10:06:20AM -0700, Jens Axboe wrote:
>> On 11/29/18 10:04 AM, Christoph Hellwig wrote:
>>> gcc never warns about unused static inline functions.  Which makes a lot
>>> of sense at least for headers..
>>
>> Not so much for non-headers :-)
> 
> You can #include .c files too! :)

There's always that one guy...

-- 
Jens Axboe

Re: [PATCH 3/7] nvme: implement mq_ops->commit_rqs() hook

[Keith Busch]

On Thu, Nov 29, 2018 at 10:06:20AM -0700, Jens Axboe wrote:
> On 11/29/18 10:04 AM, Christoph Hellwig wrote:
> > gcc never warns about unused static inline functions.  Which makes a lot
> > of sense at least for headers..
> 
> Not so much for non-headers :-)

You can #include .c files too! :)

Re: [PATCH 3/7] nvme: implement mq_ops->commit_rqs() hook

[Jens Axboe]

On 11/29/18 10:04 AM, Christoph Hellwig wrote:
> On Thu, Nov 29, 2018 at 10:02:25AM -0700, Jens Axboe wrote:
>> On 11/29/18 8:47 AM, Christoph Hellwig wrote:
 +static inline int nvme_next_ring_index(struct nvme_queue *nvmeq, u16 
 index)
 +{
 +  if (++index == nvmeq->q_depth)
 +  return 0;
 +
 +  return index;
 +}
>>>
>>> This is unused now.
>>
>> Huh, wonder how I missed that. GCC must not have complained.
> 
> gcc never warns about unused static inline functions.  Which makes a lot
> of sense at least for headers..

Not so much for non-headers :-)

>>> Also what about this little cleanup on top?
>>
>> That looks good, I like it. With that, can I add your reviewed-by? I'll
>> run a sanity check on it first.
> 
> Reviewed-by: Christoph Hellwig 

Thanks!

-- 
Jens Axboe

Re: [PATCH 3/7] nvme: implement mq_ops->commit_rqs() hook

[Christoph Hellwig]

On Thu, Nov 29, 2018 at 10:02:25AM -0700, Jens Axboe wrote:
> On 11/29/18 8:47 AM, Christoph Hellwig wrote:
> >> +static inline int nvme_next_ring_index(struct nvme_queue *nvmeq, u16 
> >> index)
> >> +{
> >> +  if (++index == nvmeq->q_depth)
> >> +  return 0;
> >> +
> >> +  return index;
> >> +}
> > 
> > This is unused now.
> 
> Huh, wonder how I missed that. GCC must not have complained.

gcc never warns about unused static inline functions.  Which makes a lot
of sense at least for headers..

> 
> > Also what about this little cleanup on top?
> 
> That looks good, I like it. With that, can I add your reviewed-by? I'll
> run a sanity check on it first.

Reviewed-by: Christoph Hellwig 

Re: [PATCH 3/7] nvme: implement mq_ops->commit_rqs() hook

[Jens Axboe]

On 11/29/18 8:47 AM, Christoph Hellwig wrote:
>> +static inline int nvme_next_ring_index(struct nvme_queue *nvmeq, u16 index)
>> +{
>> +if (++index == nvmeq->q_depth)
>> +return 0;
>> +
>> +return index;
>> +}
> 
> This is unused now.

Huh, wonder how I missed that. GCC must not have complained.

> Also what about this little cleanup on top?

That looks good, I like it. With that, can I add your reviewed-by? I'll
run a sanity check on it first.

-- 
Jens Axboe

Re: [PATCH 3/7] nvme: implement mq_ops->commit_rqs() hook

[Christoph Hellwig]

> +static inline int nvme_next_ring_index(struct nvme_queue *nvmeq, u16 index)
> +{
> + if (++index == nvmeq->q_depth)
> + return 0;
> +
> + return index;
> +}

This is unused now.

Also what about this little cleanup on top?

diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index 42472bd0cfed..527907aa6903 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -523,22 +523,26 @@ static int nvme_pci_map_queues(struct blk_mq_tag_set *set)
return 0;
 }
 
-static inline void nvme_write_sq_db(struct nvme_queue *nvmeq)
+/*
+ * Write sq tail if we are asked to, or if the next command would wrap.
+ */
+static inline void nvme_write_sq_db(struct nvme_queue *nvmeq, bool write_sq)
 {
+   if (!write_sq) {
+   u16 next_tail = nvmeq->sq_tail + 1;
+
+   if (next_tail == nvmeq->q_depth)
+   next_tail = 0;
+   if (next_tail != nvmeq->last_sq_tail)
+   return;
+   }
+
if (nvme_dbbuf_update_and_check_event(nvmeq->sq_tail,
nvmeq->dbbuf_sq_db, nvmeq->dbbuf_sq_ei))
writel(nvmeq->sq_tail, nvmeq->q_db);
nvmeq->last_sq_tail = nvmeq->sq_tail;
 }
 
-static inline int nvme_next_ring_index(struct nvme_queue *nvmeq, u16 index)
-{
-   if (++index == nvmeq->q_depth)
-   return 0;
-
-   return index;
-}
-
 /**
  * nvme_submit_cmd() - Copy a command into a queue and ring the doorbell
  * @nvmeq: The queue to use
@@ -548,24 +552,11 @@ static inline int nvme_next_ring_index(struct nvme_queue 
*nvmeq, u16 index)
 static void nvme_submit_cmd(struct nvme_queue *nvmeq, struct nvme_command *cmd,
bool write_sq)
 {
-   u16 next_tail;
-
spin_lock(&nvmeq->sq_lock);
-
memcpy(&nvmeq->sq_cmds[nvmeq->sq_tail], cmd, sizeof(*cmd));
-
if (++nvmeq->sq_tail == nvmeq->q_depth)
nvmeq->sq_tail = 0;
-
-   next_tail = nvmeq->sq_tail + 1;
-   if (next_tail == nvmeq->q_depth)
-   next_tail = 0;
-
-   /*
-* Write sq tail if we have to, OR if the next command would wrap
-*/
-   if (write_sq || next_tail == nvmeq->last_sq_tail)
-   nvme_write_sq_db(nvmeq);
+   nvme_write_sq_db(nvmeq, write_sq);
spin_unlock(&nvmeq->sq_lock);
 }
 
@@ -575,7 +566,7 @@ static void nvme_commit_rqs(struct blk_mq_hw_ctx *hctx)
 
spin_lock(&nvmeq->sq_lock);
if (nvmeq->sq_tail != nvmeq->last_sq_tail)
-   nvme_write_sq_db(nvmeq);
+   nvme_write_sq_db(nvmeq, true);
spin_unlock(&nvmeq->sq_lock);
 }