[PATCH] nvme: use lighter smp barriers in nvme_irq

[PATCH] nvme: use lighter smp barriers in nvme_irq

[Heiner Kallweit]

Based on the description we should be fine using the less heavy-weight
smp barriers here. On x86 this would be compiler barriers only.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/nvme/host/pci.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index 7b6632c00..59fee4a60 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -1066,10 +1066,10 @@ static irqreturn_t nvme_irq(int irq, void *data)
 	 * The rmb/wmb pair ensures we see all updates from a previous run of
 	 * the irq handler, even if that was on another CPU.
 	 */
-	rmb();
+	smp_rmb();
 	if (nvme_process_cq(nvmeq))
 		ret = IRQ_HANDLED;
-	wmb();
+	smp_wmb();
 
 	return ret;
 }
-- 
2.30.1


_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH] nvme: use lighter smp barriers in nvme_irq

[Chaitanya Kulkarni]

On 2/14/21 08:30, Heiner Kallweit wrote:
> Based on the description we should be fine using the less heavy-weight
> smp barriers here. On x86 this would be compiler barriers only.
>
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
Can you share performance numbers ?

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH] nvme: use lighter smp barriers in nvme_irq

[Heiner Kallweit]

On 17.02.2021 00:59, Chaitanya Kulkarni wrote:
> On 2/14/21 08:30, Heiner Kallweit wrote:
>> Based on the description we should be fine using the less heavy-weight
>> smp barriers here. On x86 this would be compiler barriers only.
>>
>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> Can you share performance numbers ?
> 

I stumbled across this code piece by chance and don't have hw to test it.
Having said that the change is based on code inspection only.
Unfortunately the barrier comment is very generic and doesn't mention
a problematic scenario. Also the commit message of 3a7afd8ee42a doesn't
mention a potential race. Most likely the barrier can be removed
completely. Helpful would be if someone could explain the potential race
in detail, means which reordering / variable accesses could be racy.
Then we would have a basis to talk about READ_ONCE/WRITE_ONCE vs.
smp barrier vs. dma barrier vs. full barrier.

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH] nvme: use lighter smp barriers in nvme_irq

[Chaitanya Kulkarni]

On 2/16/21 23:26, Heiner Kallweit wrote:
> On 17.02.2021 00:59, Chaitanya Kulkarni wrote:
>> On 2/14/21 08:30, Heiner Kallweit wrote:
>>> Based on the description we should be fine using the less heavy-weight
>>> smp barriers here. On x86 this would be compiler barriers only.
>>>
>>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>> Can you share performance numbers ?
>>
> I stumbled across this code piece by chance and don't have hw to test it.
> Having said that the change is based on code inspection only.
> Unfortunately the barrier comment is very generic and doesn't mention
> a problematic scenario. Also the commit message of 3a7afd8ee42a doesn't
> mention a potential race. Most likely the barrier can be removed
> completely. Helpful would be if someone could explain the potential race
> in detail, means which reordering / variable accesses could be racy.
> Then we would have a basis to talk about READ_ONCE/WRITE_ONCE vs.
> smp barrier vs. dma barrier vs. full barrier.
>

For these question you need to add commit Author to original email, as
far as
I know any performance improvement needs to be backed by data, unless there
is an exception, in this case I'm not if it is.


_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH] nvme: use lighter smp barriers in nvme_irq

[Keith Busch]

On Wed, Feb 17, 2021 at 08:26:30AM +0100, Heiner Kallweit wrote:
> On 17.02.2021 00:59, Chaitanya Kulkarni wrote:
> > On 2/14/21 08:30, Heiner Kallweit wrote:
> >> Based on the description we should be fine using the less heavy-weight
> >> smp barriers here. On x86 this would be compiler barriers only.
> >>
> >> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> > Can you share performance numbers ?
> > 
> 
> I stumbled across this code piece by chance and don't have hw to test it.
> Having said that the change is based on code inspection only.
> Unfortunately the barrier comment is very generic and doesn't mention
> a problematic scenario. Also the commit message of 3a7afd8ee42a doesn't
> mention a potential race. Most likely the barrier can be removed
> completely. Helpful would be if someone could explain the potential race
> in detail, means which reordering / variable accesses could be racy.
> Then we would have a basis to talk about READ_ONCE/WRITE_ONCE vs.
> smp barrier vs. dma barrier vs. full barrier.

The variables the driver was protecting no longer exist, so I also agree
the barriers should not be necessary.

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH] nvme: use lighter smp barriers in nvme_irq

[Chaitanya Kulkarni]

Keith,

On 2/17/21 19:28, Keith Busch wrote:
> On Wed, Feb 17, 2021 at 08:26:30AM +0100, Heiner Kallweit wrote:
>> On 17.02.2021 00:59, Chaitanya Kulkarni wrote:
>>> On 2/14/21 08:30, Heiner Kallweit wrote:
>>>> Based on the description we should be fine using the less heavy-weight
>>>> smp barriers here. On x86 this would be compiler barriers only.
>>>>
>>>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>>> Can you share performance numbers ?
>>>
>> I stumbled across this code piece by chance and don't have hw to test it.
>> Having said that the change is based on code inspection only.
>> Unfortunately the barrier comment is very generic and doesn't mention
>> a problematic scenario. Also the commit message of 3a7afd8ee42a doesn't
>> mention a potential race. Most likely the barrier can be removed
>> completely. Helpful would be if someone could explain the potential race
>> in detail, means which reordering / variable accesses could be racy.
>> Then we would have a basis to talk about READ_ONCE/WRITE_ONCE vs.
>> smp barrier vs. dma barrier vs. full barrier.
> The variables the driver was protecting no longer exist, so I also agree
> the barriers should not be necessary.
>

Are you saying something like following is needed ?

From a3a73bd3943b479f82ff00582baf2c37c1afd8e8 Mon Sep 17 00:00:00 2001
From: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
Date: Wed, 17 Feb 2021 20:01:37 -0800
Subject: [PATCH] nvme-pci: remove the barriers

The variable which was protected by the barriers is removed in
The commit f6c4d97b0d82 (" nvme/pci: Remove last_cq_head").

Remove the barriers which was protecting the variable.

Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
---
 drivers/nvme/host/pci.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index 0045c5edf629..3729775f6a8a 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -1062,14 +1062,8 @@ static irqreturn_t nvme_irq(int irq, void *data)
     struct nvme_queue *nvmeq = data;
     irqreturn_t ret = IRQ_NONE;
 
-    /*
-     * The rmb/wmb pair ensures we see all updates from a previous run of
-     * the irq handler, even if that was on another CPU.
-     */
-    rmb();
     if (nvme_process_cq(nvmeq))
         ret = IRQ_HANDLED;
-    wmb();
 
     return ret;
 }
-- 
2.22.1

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH] nvme: use lighter smp barriers in nvme_irq

[Keith Busch]

On Thu, Feb 18, 2021 at 04:06:35AM +0000, Chaitanya Kulkarni wrote:
> On 2/17/21 19:28, Keith Busch wrote:
> > The variables the driver was protecting no longer exist, so I also agree
> > the barriers should not be necessary.
> >
> 
> Are you saying something like following is needed ?

Right, I'm just saying the barriers don't appear to be necessary, so
they can go away as you've indicated below.
 
> From a3a73bd3943b479f82ff00582baf2c37c1afd8e8 Mon Sep 17 00:00:00 2001
> From: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
> Date: Wed, 17 Feb 2021 20:01:37 -0800
> Subject: [PATCH] nvme-pci: remove the barriers
> 
> The variable which was protected by the barriers is removed in
> The commit f6c4d97b0d82 (" nvme/pci: Remove last_cq_head").
> 
> Remove the barriers which was protecting the variable.
> 
> Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
> ---
>  drivers/nvme/host/pci.c | 6 ------
>  1 file changed, 6 deletions(-)
> 
> diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
> index 0045c5edf629..3729775f6a8a 100644
> --- a/drivers/nvme/host/pci.c
> +++ b/drivers/nvme/host/pci.c
> @@ -1062,14 +1062,8 @@ static irqreturn_t nvme_irq(int irq, void *data)
>      struct nvme_queue *nvmeq = data;
>      irqreturn_t ret = IRQ_NONE;
>  
> -    /*
> -     * The rmb/wmb pair ensures we see all updates from a previous run of
> -     * the irq handler, even if that was on another CPU.
> -     */
> -    rmb();
>      if (nvme_process_cq(nvmeq))
>          ret = IRQ_HANDLED;
> -    wmb();
>  
>      return ret;
>  }
> -- 
> 2.22.1

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme