Re: [PATCH] NVMe: Avoid interrupt disable during queue init.

2015-05-22 Thread Parav Pandit
On Fri, May 22, 2015 at 11:17 PM, Keith Busch wrote: > On Fri, 22 May 2015, Parav Pandit wrote: >> >> I agree to it that nvmeq won't be null after mb(); That alone is not >> sufficient. >> >> What I have proposed in previous email is, >> >> Converting, >> >> struct nvme_queue *nvmeq =

Re: [PATCH] NVMe: Avoid interrupt disable during queue init.

2015-05-22 Thread Keith Busch
On Fri, 22 May 2015, Parav Pandit wrote: I agree to it that nvmeq won't be null after mb(); That alone is not sufficient. What I have proposed in previous email is, Converting, struct nvme_queue *nvmeq = dev->queues[i]; if (!nvmeq) continue; spin_lock_irq(nvmeq->q_lock); to replace with,

Re: [PATCH] NVMe: Avoid interrupt disable during queue init.

2015-05-22 Thread Parav Pandit
On Fri, May 22, 2015 at 10:37 PM, Keith Busch wrote: > On Fri, 22 May 2015, Parav Pandit wrote: >> >> On Fri, May 22, 2015 at 9:53 PM, Keith Busch >> wrote: >>> >>> A memory barrier before incrementing the dev->queue_count (and assigning >>> the pointer in the array before that) should address

Re: [PATCH] NVMe: Avoid interrupt disable during queue init.

2015-05-22 Thread Keith Busch
On Fri, 22 May 2015, Parav Pandit wrote: On Fri, May 22, 2015 at 9:53 PM, Keith Busch wrote: A memory barrier before incrementing the dev->queue_count (and assigning the pointer in the array before that) should address this concern. Sure. mb() will solve the publisher side problem. RCU is

Re: [PATCH] NVMe: Avoid interrupt disable during queue init.

2015-05-22 Thread Parav Pandit
On Fri, May 22, 2015 at 9:53 PM, Keith Busch wrote: > On Fri, 22 May 2015, Parav Pandit wrote: >> >> During normal positive path probe, >> (a) device is added to dev_list in nvme_dev_start() >> (b) nvme_kthread got created, which will eventually refers to >> dev->queues[qid] to check for NULL. >>

Re: [PATCH] NVMe: Avoid interrupt disable during queue init.

2015-05-22 Thread Keith Busch
On Fri, 22 May 2015, Parav Pandit wrote: During normal positive path probe, (a) device is added to dev_list in nvme_dev_start() (b) nvme_kthread got created, which will eventually refers to dev->queues[qid] to check for NULL. (c) dev_start() worker thread has started probing device and creating

Re: [PATCH] NVMe: Avoid interrupt disable during queue init.

2015-05-22 Thread Parav Pandit
On Fri, May 22, 2015 at 8:41 PM, Keith Busch wrote: > On Fri, 22 May 2015, Parav Pandit wrote: >> >> On Fri, May 22, 2015 at 8:18 PM, Keith Busch >> wrote: >>> >>> The rcu protection on nvme queues was removed with the blk-mq conversion >>> as we rely on that layer for h/w access. >> >> >> o.k.

Re: [PATCH] NVMe: Avoid interrupt disable during queue init.

2015-05-22 Thread Keith Busch
On Fri, 22 May 2015, Parav Pandit wrote: On Fri, May 22, 2015 at 8:18 PM, Keith Busch wrote: The rcu protection on nvme queues was removed with the blk-mq conversion as we rely on that layer for h/w access. o.k. But above is at level where data I/Os are not even active. Its between

Re: [PATCH] NVMe: Avoid interrupt disable during queue init.

2015-05-22 Thread Parav Pandit
On Fri, May 22, 2015 at 8:18 PM, Keith Busch wrote: > On Thu, 21 May 2015, Parav Pandit wrote: >> >> On Fri, May 22, 2015 at 1:04 AM, Keith Busch >> wrote: >>> >>> The q_lock is held to protect polling from reading inconsistent data. >> >> >> ah, yes. I can see the nvme_kthread can poll the CQ

Re: [PATCH] NVMe: Avoid interrupt disable during queue init.

2015-05-22 Thread Keith Busch
On Thu, 21 May 2015, Parav Pandit wrote: On Fri, May 22, 2015 at 1:04 AM, Keith Busch wrote: The q_lock is held to protect polling from reading inconsistent data. ah, yes. I can see the nvme_kthread can poll the CQ while its getting created through the nvme_resume(). I think this opens up

Re: [PATCH] NVMe: Avoid interrupt disable during queue init.

2015-05-22 Thread Parav Pandit
On Fri, May 22, 2015 at 8:18 PM, Keith Busch keith.bu...@intel.com wrote: On Thu, 21 May 2015, Parav Pandit wrote: On Fri, May 22, 2015 at 1:04 AM, Keith Busch keith.bu...@intel.com wrote: The q_lock is held to protect polling from reading inconsistent data. ah, yes. I can see the

Re: [PATCH] NVMe: Avoid interrupt disable during queue init.

2015-05-22 Thread Keith Busch
On Fri, 22 May 2015, Parav Pandit wrote: On Fri, May 22, 2015 at 8:18 PM, Keith Busch keith.bu...@intel.com wrote: The rcu protection on nvme queues was removed with the blk-mq conversion as we rely on that layer for h/w access. o.k. But above is at level where data I/Os are not even active.

Re: [PATCH] NVMe: Avoid interrupt disable during queue init.

2015-05-22 Thread Keith Busch
On Thu, 21 May 2015, Parav Pandit wrote: On Fri, May 22, 2015 at 1:04 AM, Keith Busch keith.bu...@intel.com wrote: The q_lock is held to protect polling from reading inconsistent data. ah, yes. I can see the nvme_kthread can poll the CQ while its getting created through the nvme_resume(). I

Re: [PATCH] NVMe: Avoid interrupt disable during queue init.

2015-05-22 Thread Keith Busch
On Fri, 22 May 2015, Parav Pandit wrote: During normal positive path probe, (a) device is added to dev_list in nvme_dev_start() (b) nvme_kthread got created, which will eventually refers to dev-queues[qid] to check for NULL. (c) dev_start() worker thread has started probing device and creating

Re: [PATCH] NVMe: Avoid interrupt disable during queue init.

2015-05-22 Thread Parav Pandit
On Fri, May 22, 2015 at 8:41 PM, Keith Busch keith.bu...@intel.com wrote: On Fri, 22 May 2015, Parav Pandit wrote: On Fri, May 22, 2015 at 8:18 PM, Keith Busch keith.bu...@intel.com wrote: The rcu protection on nvme queues was removed with the blk-mq conversion as we rely on that layer for

Re: [PATCH] NVMe: Avoid interrupt disable during queue init.

2015-05-22 Thread Parav Pandit
On Fri, May 22, 2015 at 9:53 PM, Keith Busch keith.bu...@intel.com wrote: On Fri, 22 May 2015, Parav Pandit wrote: During normal positive path probe, (a) device is added to dev_list in nvme_dev_start() (b) nvme_kthread got created, which will eventually refers to dev-queues[qid] to check for

Re: [PATCH] NVMe: Avoid interrupt disable during queue init.

2015-05-22 Thread Keith Busch
On Fri, 22 May 2015, Parav Pandit wrote: On Fri, May 22, 2015 at 9:53 PM, Keith Busch keith.bu...@intel.com wrote: A memory barrier before incrementing the dev-queue_count (and assigning the pointer in the array before that) should address this concern. Sure. mb() will solve the publisher

Re: [PATCH] NVMe: Avoid interrupt disable during queue init.

2015-05-22 Thread Keith Busch
On Fri, 22 May 2015, Parav Pandit wrote: I agree to it that nvmeq won't be null after mb(); That alone is not sufficient. What I have proposed in previous email is, Converting, struct nvme_queue *nvmeq = dev-queues[i]; if (!nvmeq) continue; spin_lock_irq(nvmeq-q_lock); to replace with,

Re: [PATCH] NVMe: Avoid interrupt disable during queue init.

2015-05-22 Thread Parav Pandit
On Fri, May 22, 2015 at 11:17 PM, Keith Busch keith.bu...@intel.com wrote: On Fri, 22 May 2015, Parav Pandit wrote: I agree to it that nvmeq won't be null after mb(); That alone is not sufficient. What I have proposed in previous email is, Converting, struct nvme_queue *nvmeq =

Re: [PATCH] NVMe: Avoid interrupt disable during queue init.

2015-05-22 Thread Parav Pandit
On Fri, May 22, 2015 at 10:37 PM, Keith Busch keith.bu...@intel.com wrote: On Fri, 22 May 2015, Parav Pandit wrote: On Fri, May 22, 2015 at 9:53 PM, Keith Busch keith.bu...@intel.com wrote: A memory barrier before incrementing the dev-queue_count (and assigning the pointer in the array

Re: [PATCH] NVMe: Avoid interrupt disable during queue init.

2015-05-21 Thread Parav Pandit
On Fri, May 22, 2015 at 1:04 AM, Keith Busch wrote: > On Thu, 21 May 2015, Parav Pandit wrote: >> >> Avoid diabling interrupt and holding q_lock for the queue >> which is just getting initialized. >> >> With this change, online_queues is also incremented without >> lock during queue setup stage.

Re: [PATCH] NVMe: Avoid interrupt disable during queue init.

2015-05-21 Thread Keith Busch
On Thu, 21 May 2015, Parav Pandit wrote: Avoid diabling interrupt and holding q_lock for the queue which is just getting initialized. With this change, online_queues is also incremented without lock during queue setup stage. if Power management nvme_suspend() kicks in during queue setup time,

Re: [PATCH] NVMe: Avoid interrupt disable during queue init.

2015-05-21 Thread Parav Pandit
On Fri, May 22, 2015 at 12:09 AM, Jens Axboe wrote: > On 05/21/2015 06:12 PM, Parav Pandit wrote: >> >> Avoid diabling interrupt and holding q_lock for the queue >> which is just getting initialized. >> >> With this change, online_queues is also incremented without >> lock during queue setup

Re: [PATCH] NVMe: Avoid interrupt disable during queue init.

2015-05-21 Thread Jens Axboe
On 05/21/2015 06:12 PM, Parav Pandit wrote: Avoid diabling interrupt and holding q_lock for the queue which is just getting initialized. With this change, online_queues is also incremented without lock during queue setup stage. if Power management nvme_suspend() kicks in during queue setup

[PATCH] NVMe: Avoid interrupt disable during queue init.

2015-05-21 Thread Parav Pandit
Avoid diabling interrupt and holding q_lock for the queue which is just getting initialized. With this change, online_queues is also incremented without lock during queue setup stage. if Power management nvme_suspend() kicks in during queue setup time, per nvmeq based q_lock spinlock cannot

Re: [PATCH] NVMe: Avoid interrupt disable during queue init.

2015-05-21 Thread Jens Axboe
On 05/21/2015 06:12 PM, Parav Pandit wrote: Avoid diabling interrupt and holding q_lock for the queue which is just getting initialized. With this change, online_queues is also incremented without lock during queue setup stage. if Power management nvme_suspend() kicks in during queue setup

Re: [PATCH] NVMe: Avoid interrupt disable during queue init.

2015-05-21 Thread Parav Pandit
On Fri, May 22, 2015 at 1:04 AM, Keith Busch keith.bu...@intel.com wrote: On Thu, 21 May 2015, Parav Pandit wrote: Avoid diabling interrupt and holding q_lock for the queue which is just getting initialized. With this change, online_queues is also incremented without lock during queue setup

Re: [PATCH] NVMe: Avoid interrupt disable during queue init.

2015-05-21 Thread Keith Busch
On Thu, 21 May 2015, Parav Pandit wrote: Avoid diabling interrupt and holding q_lock for the queue which is just getting initialized. With this change, online_queues is also incremented without lock during queue setup stage. if Power management nvme_suspend() kicks in during queue setup time,

[PATCH] NVMe: Avoid interrupt disable during queue init.

2015-05-21 Thread Parav Pandit
Avoid diabling interrupt and holding q_lock for the queue which is just getting initialized. With this change, online_queues is also incremented without lock during queue setup stage. if Power management nvme_suspend() kicks in during queue setup time, per nvmeq based q_lock spinlock cannot

Re: [PATCH] NVMe: Avoid interrupt disable during queue init.

2015-05-21 Thread Parav Pandit
On Fri, May 22, 2015 at 12:09 AM, Jens Axboe ax...@kernel.dk wrote: On 05/21/2015 06:12 PM, Parav Pandit wrote: Avoid diabling interrupt and holding q_lock for the queue which is just getting initialized. With this change, online_queues is also incremented without lock during queue setup