linux-block.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/2] block/nvme: Fix DMA-noncoherent platforms support
@ 2022-09-09 19:19 Serge Semin
  2022-09-09 19:19 ` [PATCH 1/2] nvme-hwmon: Cache-line-align the NVME SMART log-buffer Serge Semin
  2022-09-09 19:19 ` [PATCH 2/2] block: sed-opal: Cache-line-align the cmd/resp buffers Serge Semin
  0 siblings, 2 replies; 17+ messages in thread
From: Serge Semin @ 2022-09-09 19:19 UTC (permalink / raw)
  To: Jonathan Derrick, Revanth Rajashekar, Jens Axboe, Keith Busch,
	Jens Axboe, Christoph Hellwig, Sagi Grimberg
  Cc: Serge Semin, Serge Semin, Alexey Malahov, Pavel Parkhomenko,
	Thomas Bogendoerfer, linux-nvme, linux-block, linux-kernel

Our SoC doesn't have the CPU caches coherent on DMA's. After getting the
kernel updated to the 6.0-rcX version we've discovered a problem with the
NVME hwmon probe. It turned out that the root cause of it was connected
with the cache-line-unaligned buffer passed to the DMA-engine. Due to the
cache-invalidation performed on the buffer mapping stage a part of the
structure the buffer was embedded to was lost. Here we suggest to fix the
problem just by aligning the buffer accordingly as the
Documentation/core-api/dma-api.rst document requires. (See the
corresponding patch log for more details.)

A potential root of a similar problem has been detected in the sed-opal
driver too. Even though we have not got any difficulties connected with
that part we still suggest to fix that in the same way as it is done for
the NVME hwmon driver.

Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
Cc: Alexey Malahov <Alexey.Malahov@baikalelectronics.ru>
Cc: Pavel Parkhomenko <Pavel.Parkhomenko@baikalelectronics.ru>
Cc: Thomas Bogendoerfer <tsbogend@alpha.franken.de>
Cc: linux-nvme@lists.infradead.org
Cc: linux-block@vger.kernel.org
Cc: linux-kernel@vger.kernel.org

Serge Semin (2):
  nvme-hwmon: Cache-line-align the NVME SMART log-buffer
  block: sed-opal: Cache-line-align the cmd/resp buffers

 block/sed-opal.c          | 5 +++--
 drivers/nvme/host/hwmon.c | 3 ++-
 2 files changed, 5 insertions(+), 3 deletions(-)

-- 
2.37.2


^ permalink raw reply	[flat|nested] 17+ messages in thread

* [PATCH 1/2] nvme-hwmon: Cache-line-align the NVME SMART log-buffer
  2022-09-09 19:19 [PATCH 0/2] block/nvme: Fix DMA-noncoherent platforms support Serge Semin
@ 2022-09-09 19:19 ` Serge Semin
  2022-09-09 19:42   ` Keith Busch
                     ` (2 more replies)
  2022-09-09 19:19 ` [PATCH 2/2] block: sed-opal: Cache-line-align the cmd/resp buffers Serge Semin
  1 sibling, 3 replies; 17+ messages in thread
From: Serge Semin @ 2022-09-09 19:19 UTC (permalink / raw)
  To: Jonathan Derrick, Revanth Rajashekar, Jens Axboe, Keith Busch,
	Jens Axboe, Christoph Hellwig, Sagi Grimberg, Guenter Roeck
  Cc: Serge Semin, Serge Semin, Alexey Malahov, Pavel Parkhomenko,
	Thomas Bogendoerfer, linux-nvme, linux-block, linux-kernel

Recent commit 52fde2c07da6 ("nvme: set dma alignment to dword") has caused
a regression on our platform. It turned out that the nvme_get_log() method
invocation caused the nvme_hwmon_data structure instance corruption. In
particular the nvme_hwmon_data.ctrl pointer was overwritten either with
zeros or with garbage. After some researches we discovered that the
problem happened even before the actual NVME DMA execution, but during the
buffer mapping. Since our platform was DMA-noncoherent the mapping implied
the cache-lines invalidations or write-backs depending on the
DMA-direction parameter. In case of the NVME SMART log getting the DMA
was performed from-device-to-memory, thus the cache-invalidation was
activated during the buffer mapping. Since the log-buffer wasn't
cache-line aligned the cache-invalidation caused the neighbour data
discard. The neighbouring data turned to be the data surrounding the
buffer in the framework of the nvme_hwmon_data structure.

In order to fix that we need to make sure that the whole log-buffer is
defined within the cache-line-aligned memory region so the
cache-invalidation procedure wouldn't involve the adjacent data. By doing
so we not only get rid from the denoted problem but also fulfill the
requirement explicitly described in [1].

After a deeper researches we found out that the denoted commit wasn't a
root cause of the problem. It just revealed the invalidity by activating
the DMA-based NVME SMART log getting performed in the framework of the
NVME hwmon driver. The problem was here since the initial commit of the
driver.

[1] Documentation/core-api/dma-api.rst

Fixes: 400b6a7b13a3 ("nvme: Add hardware monitoring support")
Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>

---

Folks, I've thoroughly studied the whole NVME subsystem looking for
similar problems. Turned out there is one more place which may cause the
same issue. It's connected with the opal_dev.{cmd,req} buffers passed to
the nvme_sec_submit() method. The rest of the buffers involved in the NVME
DMA are either allocated by kmalloc (must be cache-line-aligned by design)
or bounced-buffered if allocated on the stack (see the blk_rq_map_kern()
method implementation).
---
 drivers/nvme/host/hwmon.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/nvme/host/hwmon.c b/drivers/nvme/host/hwmon.c
index 0a586d712920..94192ab7a02d 100644
--- a/drivers/nvme/host/hwmon.c
+++ b/drivers/nvme/host/hwmon.c
@@ -10,9 +10,10 @@
 
 #include "nvme.h"
 
+/* DMA-noncoherent platforms require the cache-aligned buffers */
 struct nvme_hwmon_data {
+	struct nvme_smart_log log ____cacheline_aligned;
 	struct nvme_ctrl *ctrl;
-	struct nvme_smart_log log;
 	struct mutex read_lock;
 };
 
-- 
2.37.2


^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH 2/2] block: sed-opal: Cache-line-align the cmd/resp buffers
  2022-09-09 19:19 [PATCH 0/2] block/nvme: Fix DMA-noncoherent platforms support Serge Semin
  2022-09-09 19:19 ` [PATCH 1/2] nvme-hwmon: Cache-line-align the NVME SMART log-buffer Serge Semin
@ 2022-09-09 19:19 ` Serge Semin
  2022-09-10  5:32   ` Christoph Hellwig
  1 sibling, 1 reply; 17+ messages in thread
From: Serge Semin @ 2022-09-09 19:19 UTC (permalink / raw)
  To: Jonathan Derrick, Revanth Rajashekar, Jens Axboe, Keith Busch,
	Jens Axboe, Christoph Hellwig, Sagi Grimberg, Rafael Antognolli,
	Scott Bauer
  Cc: Serge Semin, Serge Semin, Alexey Malahov, Pavel Parkhomenko,
	Thomas Bogendoerfer, linux-nvme, linux-block, linux-kernel

In accordance with [1] the DMA-able memory buffers must be
cacheline-aligned otherwise the cache writing-back and invalidation
performed during the mapping may cause the adjacent data being lost. It's
specifically required for the DMA-noncoherent platforms. Seeing the
opal_dev.{cmd,resp} buffers are used for DMAs in the NVME and SCSI/SD
drivers in framework of the nvme_sec_submit() and sd_sec_submit() methods
respectively we must make sure the passed buffers are cacheline-aligned to
prevent the denoted problem.

[1] Documentation/core-api/dma-api.rst

Fixes: 455a7b238cd6 ("block: Add Sed-opal library")
Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
---
 block/sed-opal.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/block/sed-opal.c b/block/sed-opal.c
index 9700197000f2..222acbd1f03a 100644
--- a/block/sed-opal.c
+++ b/block/sed-opal.c
@@ -73,6 +73,7 @@ struct parsed_resp {
 	struct opal_resp_tok toks[MAX_TOKS];
 };
 
+/* Presumably DMA-able buffers must be cache-aligned */
 struct opal_dev {
 	bool supported;
 	bool mbr_enabled;
@@ -88,8 +89,8 @@ struct opal_dev {
 	u64 lowest_lba;
 
 	size_t pos;
-	u8 cmd[IO_BUFFER_LENGTH];
-	u8 resp[IO_BUFFER_LENGTH];
+	u8 cmd[IO_BUFFER_LENGTH] ____cacheline_aligned;
+	u8 resp[IO_BUFFER_LENGTH] ____cacheline_aligned;
 
 	struct parsed_resp parsed;
 	size_t prev_d_len;
-- 
2.37.2


^ permalink raw reply related	[flat|nested] 17+ messages in thread

* Re: [PATCH 1/2] nvme-hwmon: Cache-line-align the NVME SMART log-buffer
  2022-09-09 19:19 ` [PATCH 1/2] nvme-hwmon: Cache-line-align the NVME SMART log-buffer Serge Semin
@ 2022-09-09 19:42   ` Keith Busch
  2022-09-09 20:53     ` Serge Semin
  2022-09-09 20:36   ` Guenter Roeck
  2022-09-10  5:30   ` Christoph Hellwig
  2 siblings, 1 reply; 17+ messages in thread
From: Keith Busch @ 2022-09-09 19:42 UTC (permalink / raw)
  To: Serge Semin
  Cc: Jonathan Derrick, Revanth Rajashekar, Jens Axboe, Jens Axboe,
	Christoph Hellwig, Sagi Grimberg, Guenter Roeck, Serge Semin,
	Alexey Malahov, Pavel Parkhomenko, Thomas Bogendoerfer,
	linux-nvme, linux-block, linux-kernel

On Fri, Sep 09, 2022 at 10:19:15PM +0300, Serge Semin wrote:
> Recent commit 52fde2c07da6 ("nvme: set dma alignment to dword") has caused
> 
> Folks, I've thoroughly studied the whole NVME subsystem looking for
> similar problems. Turned out there is one more place which may cause the
> same issue. It's connected with the opal_dev.{cmd,req} buffers passed to
> the nvme_sec_submit() method. The rest of the buffers involved in the NVME
> DMA are either allocated by kmalloc (must be cache-line-aligned by design)
> or bounced-buffered if allocated on the stack (see the blk_rq_map_kern()
> method implementation).

What about user space addresses? We can map those with cacheline offsets.

> ---
>  drivers/nvme/host/hwmon.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/nvme/host/hwmon.c b/drivers/nvme/host/hwmon.c
> index 0a586d712920..94192ab7a02d 100644
> --- a/drivers/nvme/host/hwmon.c
> +++ b/drivers/nvme/host/hwmon.c
> @@ -10,9 +10,10 @@
>  
>  #include "nvme.h"
>  
> +/* DMA-noncoherent platforms require the cache-aligned buffers */
>  struct nvme_hwmon_data {
> +	struct nvme_smart_log log ____cacheline_aligned;
>  	struct nvme_ctrl *ctrl;
> -	struct nvme_smart_log log;

So this by chance happened to work before 52fde2c07da6 because the field
started at a 4-byte offset on your arch?

The change looks good.

Reviewed-by: Keith Busch <kbusch@kernel.org>

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 1/2] nvme-hwmon: Cache-line-align the NVME SMART log-buffer
  2022-09-09 19:19 ` [PATCH 1/2] nvme-hwmon: Cache-line-align the NVME SMART log-buffer Serge Semin
  2022-09-09 19:42   ` Keith Busch
@ 2022-09-09 20:36   ` Guenter Roeck
  2022-09-10  5:30   ` Christoph Hellwig
  2 siblings, 0 replies; 17+ messages in thread
From: Guenter Roeck @ 2022-09-09 20:36 UTC (permalink / raw)
  To: Serge Semin, Jonathan Derrick, Revanth Rajashekar, Jens Axboe,
	Keith Busch, Jens Axboe, Christoph Hellwig, Sagi Grimberg
  Cc: Serge Semin, Alexey Malahov, Pavel Parkhomenko,
	Thomas Bogendoerfer, linux-nvme, linux-block, linux-kernel

On 9/9/22 12:19, Serge Semin wrote:
> Recent commit 52fde2c07da6 ("nvme: set dma alignment to dword") has caused
> a regression on our platform. It turned out that the nvme_get_log() method
> invocation caused the nvme_hwmon_data structure instance corruption. In
> particular the nvme_hwmon_data.ctrl pointer was overwritten either with
> zeros or with garbage. After some researches we discovered that the
> problem happened even before the actual NVME DMA execution, but during the
> buffer mapping. Since our platform was DMA-noncoherent the mapping implied
> the cache-lines invalidations or write-backs depending on the
> DMA-direction parameter. In case of the NVME SMART log getting the DMA
> was performed from-device-to-memory, thus the cache-invalidation was
> activated during the buffer mapping. Since the log-buffer wasn't
> cache-line aligned the cache-invalidation caused the neighbour data
> discard. The neighbouring data turned to be the data surrounding the
> buffer in the framework of the nvme_hwmon_data structure.
> 
> In order to fix that we need to make sure that the whole log-buffer is
> defined within the cache-line-aligned memory region so the
> cache-invalidation procedure wouldn't involve the adjacent data. By doing
> so we not only get rid from the denoted problem but also fulfill the
> requirement explicitly described in [1].
> 
> After a deeper researches we found out that the denoted commit wasn't a
> root cause of the problem. It just revealed the invalidity by activating
> the DMA-based NVME SMART log getting performed in the framework of the
> NVME hwmon driver. The problem was here since the initial commit of the
> driver.
> 
> [1] Documentation/core-api/dma-api.rst
> 
> Fixes: 400b6a7b13a3 ("nvme: Add hardware monitoring support")
> Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>

Thanks for tracking this down and for the fix.

Reviewed-by: Guenter Roeck <linux@roeck-us.net>

Guenter

> 
> ---
> 
> Folks, I've thoroughly studied the whole NVME subsystem looking for
> similar problems. Turned out there is one more place which may cause the
> same issue. It's connected with the opal_dev.{cmd,req} buffers passed to
> the nvme_sec_submit() method. The rest of the buffers involved in the NVME
> DMA are either allocated by kmalloc (must be cache-line-aligned by design)
> or bounced-buffered if allocated on the stack (see the blk_rq_map_kern()
> method implementation).
> ---
>   drivers/nvme/host/hwmon.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/nvme/host/hwmon.c b/drivers/nvme/host/hwmon.c
> index 0a586d712920..94192ab7a02d 100644
> --- a/drivers/nvme/host/hwmon.c
> +++ b/drivers/nvme/host/hwmon.c
> @@ -10,9 +10,10 @@
>   
>   #include "nvme.h"
>   
> +/* DMA-noncoherent platforms require the cache-aligned buffers */
>   struct nvme_hwmon_data {
> +	struct nvme_smart_log log ____cacheline_aligned;
>   	struct nvme_ctrl *ctrl;
> -	struct nvme_smart_log log;
>   	struct mutex read_lock;
>   };
>   


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 1/2] nvme-hwmon: Cache-line-align the NVME SMART log-buffer
  2022-09-09 19:42   ` Keith Busch
@ 2022-09-09 20:53     ` Serge Semin
  0 siblings, 0 replies; 17+ messages in thread
From: Serge Semin @ 2022-09-09 20:53 UTC (permalink / raw)
  To: Keith Busch
  Cc: Serge Semin, Jonathan Derrick, Revanth Rajashekar, Jens Axboe,
	Jens Axboe, Christoph Hellwig, Sagi Grimberg, Guenter Roeck,
	Alexey Malahov, Pavel Parkhomenko, Thomas Bogendoerfer,
	linux-nvme, linux-block, linux-kernel

On Fri, Sep 09, 2022 at 01:42:34PM -0600, Keith Busch wrote:
> On Fri, Sep 09, 2022 at 10:19:15PM +0300, Serge Semin wrote:
> > Recent commit 52fde2c07da6 ("nvme: set dma alignment to dword") has caused
> > 
> > Folks, I've thoroughly studied the whole NVME subsystem looking for
> > similar problems. Turned out there is one more place which may cause the
> > same issue. It's connected with the opal_dev.{cmd,req} buffers passed to
> > the nvme_sec_submit() method. The rest of the buffers involved in the NVME
> > DMA are either allocated by kmalloc (must be cache-line-aligned by design)
> > or bounced-buffered if allocated on the stack (see the blk_rq_map_kern()
> > method implementation).
> 

> What about user space addresses?

Reasonable question. Alas I haven't researched the user-space part as
much thorough. What I can say for sure that we haven't detected any
unaligned buffers passed to the DMA-mapping procedure other than the
ones denoted in this patch and in the next one. So to speak so far
none of the NVME-involved user-space buffers have had unaligned offset
in the physical address space. I have merged in the next patch in our
local kernel tree:
https://patchwork.linux-mips.org/project/linux-mips/patch/20161125184611.28396-3-paul.burton@imgtec.com/
So if an unaligned buffer was passed we would have immediately got it
detected.

> We can map those with cacheline offsets.

If we could do that easily it would have been great. But I don't see
an easy way out. AFAICS we'll need to fix the blk_rq_map_user_iov()
method so one would CPU-based copy the unaligned part of the buffer
and perform the DMA-required operations with the rest of it. Do you
have any better suggestion in mind?

> 
> > ---
> >  drivers/nvme/host/hwmon.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/nvme/host/hwmon.c b/drivers/nvme/host/hwmon.c
> > index 0a586d712920..94192ab7a02d 100644
> > --- a/drivers/nvme/host/hwmon.c
> > +++ b/drivers/nvme/host/hwmon.c
> > @@ -10,9 +10,10 @@
> >  
> >  #include "nvme.h"
> >  
> > +/* DMA-noncoherent platforms require the cache-aligned buffers */
> >  struct nvme_hwmon_data {
> > +	struct nvme_smart_log log ____cacheline_aligned;
> >  	struct nvme_ctrl *ctrl;
> > -	struct nvme_smart_log log;
> 

> So this by chance happened to work before 52fde2c07da6 because the field
> started at a 4-byte offset on your arch?

Correct. The offset is 4-bytes indeed so the log-field base address is
4-bytes aligned. Due to that the bounce-buffer used to be used for the
NVME SMART log getting. Since the denoted commit the log-buffer have
been directly used for DMA, which has revealed the problem caused by the
cache-invalidation on the buffer mapping.

> 
> The change looks good.
> 
> Reviewed-by: Keith Busch <kbusch@kernel.org>

Thanks.

-Sergey


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 1/2] nvme-hwmon: Cache-line-align the NVME SMART log-buffer
  2022-09-09 19:19 ` [PATCH 1/2] nvme-hwmon: Cache-line-align the NVME SMART log-buffer Serge Semin
  2022-09-09 19:42   ` Keith Busch
  2022-09-09 20:36   ` Guenter Roeck
@ 2022-09-10  5:30   ` Christoph Hellwig
  2022-09-10 12:35     ` Serge Semin
  2022-09-10 14:33     ` Guenter Roeck
  2 siblings, 2 replies; 17+ messages in thread
From: Christoph Hellwig @ 2022-09-10  5:30 UTC (permalink / raw)
  To: Serge Semin
  Cc: Jonathan Derrick, Revanth Rajashekar, Jens Axboe, Keith Busch,
	Jens Axboe, Christoph Hellwig, Sagi Grimberg, Guenter Roeck,
	Serge Semin, Alexey Malahov, Pavel Parkhomenko,
	Thomas Bogendoerfer, linux-nvme, linux-block, linux-kernel

I think this will work, but unless we have to I'd generally prefer
to just split dta that is DMAed into into a separate allocation.
That is, do a separate kmalloc for the nvme_smart_log structure.

Guenter, is this ok with you?

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 2/2] block: sed-opal: Cache-line-align the cmd/resp buffers
  2022-09-09 19:19 ` [PATCH 2/2] block: sed-opal: Cache-line-align the cmd/resp buffers Serge Semin
@ 2022-09-10  5:32   ` Christoph Hellwig
  2022-09-11 16:28     ` Serge Semin
  0 siblings, 1 reply; 17+ messages in thread
From: Christoph Hellwig @ 2022-09-10  5:32 UTC (permalink / raw)
  To: Serge Semin
  Cc: Jonathan Derrick, Revanth Rajashekar, Jens Axboe, Keith Busch,
	Jens Axboe, Christoph Hellwig, Sagi Grimberg, Rafael Antognolli,
	Scott Bauer, Serge Semin, Alexey Malahov, Pavel Parkhomenko,
	Thomas Bogendoerfer, linux-nvme, linux-block, linux-kernel

On Fri, Sep 09, 2022 at 10:19:16PM +0300, Serge Semin wrote:
> In accordance with [1] the DMA-able memory buffers must be
> cacheline-aligned otherwise the cache writing-back and invalidation
> performed during the mapping may cause the adjacent data being lost. It's
> specifically required for the DMA-noncoherent platforms. Seeing the
> opal_dev.{cmd,resp} buffers are used for DMAs in the NVME and SCSI/SD
> drivers in framework of the nvme_sec_submit() and sd_sec_submit() methods
> respectively we must make sure the passed buffers are cacheline-aligned to
> prevent the denoted problem.

Same comment as for the previous one, this should work, but I think
separate allocations for the DMAable buffers would document the intent
much better.  Given that the opal initialization isn't a fast path
I don't think that the overhead should matter either.

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 1/2] nvme-hwmon: Cache-line-align the NVME SMART log-buffer
  2022-09-10  5:30   ` Christoph Hellwig
@ 2022-09-10 12:35     ` Serge Semin
  2022-09-10 18:09       ` Serge Semin
  2022-09-12  8:29       ` Christoph Hellwig
  2022-09-10 14:33     ` Guenter Roeck
  1 sibling, 2 replies; 17+ messages in thread
From: Serge Semin @ 2022-09-10 12:35 UTC (permalink / raw)
  To: Christoph Hellwig
  Cc: Serge Semin, Jonathan Derrick, Revanth Rajashekar, Jens Axboe,
	Keith Busch, Jens Axboe, Sagi Grimberg, Guenter Roeck,
	Alexey Malahov, Pavel Parkhomenko, Thomas Bogendoerfer,
	linux-nvme, linux-block, linux-kernel

On Sat, Sep 10, 2022 at 07:30:45AM +0200, Christoph Hellwig wrote:
> I think this will work, but unless we have to I'd generally prefer
> to just split dta that is DMAed into into a separate allocation.
> That is, do a separate kmalloc for the nvme_smart_log structure.

Well, both approaches will solve the denoted problem. I am just
wondering why do you think that the kmalloc-ed buffer is more
preferable? IMO it is a bit less suitable since increases the memory
granularity - two kmalloc's instead of one. Moreover it makes the code
a bit more complex for the same reason of having two mallocs and two
frees. Meanwhile using the ____cacheline_aligned qualifier to prevent
the noncoherent DMA problem is a standard approach.

What would be the best solution if we had a qualifier like this:
#ifdef CONFIG_DMA_NONCOHERENT
#define ____dma_buffer ____cacheline_aligned
#else
#define ____dma_buffer
#endif
and used it instead of the direct ____cacheline_aligned utilization.

-Sergey

> 
> Guenter, is this ok with you?

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 1/2] nvme-hwmon: Cache-line-align the NVME SMART log-buffer
  2022-09-10  5:30   ` Christoph Hellwig
  2022-09-10 12:35     ` Serge Semin
@ 2022-09-10 14:33     ` Guenter Roeck
  1 sibling, 0 replies; 17+ messages in thread
From: Guenter Roeck @ 2022-09-10 14:33 UTC (permalink / raw)
  To: Christoph Hellwig, Serge Semin
  Cc: Jonathan Derrick, Revanth Rajashekar, Jens Axboe, Keith Busch,
	Jens Axboe, Sagi Grimberg, Serge Semin, Alexey Malahov,
	Pavel Parkhomenko, Thomas Bogendoerfer, linux-nvme, linux-block,
	linux-kernel

On 9/9/22 22:30, Christoph Hellwig wrote:
> I think this will work, but unless we have to I'd generally prefer
> to just split dta that is DMAed into into a separate allocation.
> That is, do a separate kmalloc for the nvme_smart_log structure.
> 
> Guenter, is this ok with you?

Seems to be a bit overkill, but sure.

Guenter

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 1/2] nvme-hwmon: Cache-line-align the NVME SMART log-buffer
  2022-09-10 12:35     ` Serge Semin
@ 2022-09-10 18:09       ` Serge Semin
  2022-09-12  8:29       ` Christoph Hellwig
  1 sibling, 0 replies; 17+ messages in thread
From: Serge Semin @ 2022-09-10 18:09 UTC (permalink / raw)
  To: Christoph Hellwig
  Cc: Serge Semin, Jonathan Derrick, Revanth Rajashekar, Jens Axboe,
	Keith Busch, Jens Axboe, Sagi Grimberg, Guenter Roeck,
	Alexey Malahov, Pavel Parkhomenko, Thomas Bogendoerfer,
	linux-nvme, linux-block, linux-kernel

On Sat, Sep 10, 2022 at 03:35:45PM +0300, Serge Semin wrote:
> On Sat, Sep 10, 2022 at 07:30:45AM +0200, Christoph Hellwig wrote:
> > I think this will work, but unless we have to I'd generally prefer
> > to just split dta that is DMAed into into a separate allocation.
> > That is, do a separate kmalloc for the nvme_smart_log structure.
> 
> Well, both approaches will solve the denoted problem. I am just
> wondering why do you think that the kmalloc-ed buffer is more
> preferable? IMO it is a bit less suitable since increases the memory

> granularity - two kmalloc's instead of one. Moreover it makes the code
  ^
  `-- I meant fragmentation of course...

> a bit more complex for the same reason of having two mallocs and two
> frees. Meanwhile using the ____cacheline_aligned qualifier to prevent
> the noncoherent DMA problem is a standard approach.
> 
> What would be the best solution if we had a qualifier like this:
> #ifdef CONFIG_DMA_NONCOHERENT
> #define ____dma_buffer ____cacheline_aligned
> #else
> #define ____dma_buffer
> #endif
> and used it instead of the direct ____cacheline_aligned utilization.
> 
> -Sergey
> 
> > 
> > Guenter, is this ok with you?

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 2/2] block: sed-opal: Cache-line-align the cmd/resp buffers
  2022-09-10  5:32   ` Christoph Hellwig
@ 2022-09-11 16:28     ` Serge Semin
  2022-09-25 22:30       ` Serge Semin
  0 siblings, 1 reply; 17+ messages in thread
From: Serge Semin @ 2022-09-11 16:28 UTC (permalink / raw)
  To: Christoph Hellwig
  Cc: Serge Semin, Jonathan Derrick, Revanth Rajashekar, Jens Axboe,
	Keith Busch, Jens Axboe, Sagi Grimberg, Rafael Antognolli,
	Scott Bauer, Alexey Malahov, Pavel Parkhomenko,
	Thomas Bogendoerfer, linux-nvme, linux-block, linux-kernel

Hello Christoph

On Sat, Sep 10, 2022 at 07:32:03AM +0200, Christoph Hellwig wrote:
> On Fri, Sep 09, 2022 at 10:19:16PM +0300, Serge Semin wrote:
> > In accordance with [1] the DMA-able memory buffers must be
> > cacheline-aligned otherwise the cache writing-back and invalidation
> > performed during the mapping may cause the adjacent data being lost. It's
> > specifically required for the DMA-noncoherent platforms. Seeing the
> > opal_dev.{cmd,resp} buffers are used for DMAs in the NVME and SCSI/SD
> > drivers in framework of the nvme_sec_submit() and sd_sec_submit() methods
> > respectively we must make sure the passed buffers are cacheline-aligned to
> > prevent the denoted problem.
>
 
> Same comment as for the previous one, this should work, but I think
> separate allocations for the DMAable buffers would document the intent
> much better.  Given that the opal initialization isn't a fast path
> I don't think that the overhead should matter either.

Thanks for the comment. I see your point. Let's hear the subsystem
maintainers out for their opinion regarding the most suitable solution
in this case. If they get to agree with you I'll resend the series
with altered fixes.

-Sergey

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 1/2] nvme-hwmon: Cache-line-align the NVME SMART log-buffer
  2022-09-10 12:35     ` Serge Semin
  2022-09-10 18:09       ` Serge Semin
@ 2022-09-12  8:29       ` Christoph Hellwig
  2022-09-25 22:23         ` Serge Semin
  1 sibling, 1 reply; 17+ messages in thread
From: Christoph Hellwig @ 2022-09-12  8:29 UTC (permalink / raw)
  To: Serge Semin
  Cc: Christoph Hellwig, Serge Semin, Jonathan Derrick,
	Revanth Rajashekar, Jens Axboe, Keith Busch, Jens Axboe,
	Sagi Grimberg, Guenter Roeck, Alexey Malahov, Pavel Parkhomenko,
	Thomas Bogendoerfer, linux-nvme, linux-block, linux-kernel

On Sat, Sep 10, 2022 at 03:35:42PM +0300, Serge Semin wrote:
> Well, both approaches will solve the denoted problem. I am just
> wondering why do you think that the kmalloc-ed buffer is more
> preferable?

Because it clearly documents the intent.  Here is one buffer that is
just a data buffer, and here is one with kernel internal structure.
The concept of embedding on-disk / on-the-wire structures into internal
stuctures always seemed rather weird and unexpected to me, as we now
need to ensure that the alignment works right on both sides.  With
the right annotations (as done in this series) this will work, but
it feels a little fragile to me.

> What would be the best solution if we had a qualifier like this:
> #ifdef CONFIG_DMA_NONCOHERENT
> #define ____dma_buffer ____cacheline_aligned
> #else
> #define ____dma_buffer
> #endif
> and used it instead of the direct ____cacheline_aligned utilization.

So independent of my preference for separate allocations, this suggested
additional would still be very useful for the places where we need
to use the alignment for performance or other reasons.  I'd use
something like __dma_alligned or similar, though.

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 1/2] nvme-hwmon: Cache-line-align the NVME SMART log-buffer
  2022-09-12  8:29       ` Christoph Hellwig
@ 2022-09-25 22:23         ` Serge Semin
  2022-09-26 14:39           ` Christoph Hellwig
  0 siblings, 1 reply; 17+ messages in thread
From: Serge Semin @ 2022-09-25 22:23 UTC (permalink / raw)
  To: Christoph Hellwig
  Cc: Serge Semin, Jonathan Derrick, Revanth Rajashekar, Jens Axboe,
	Keith Busch, Jens Axboe, Sagi Grimberg, Guenter Roeck,
	Alexey Malahov, Pavel Parkhomenko, Thomas Bogendoerfer,
	linux-nvme, linux-block, linux-kernel

Hello Christoph,

Sorry for the delay with response.

On Mon, Sep 12, 2022 at 10:29:10AM +0200, Christoph Hellwig wrote:
> On Sat, Sep 10, 2022 at 03:35:42PM +0300, Serge Semin wrote:
> > Well, both approaches will solve the denoted problem. I am just
> > wondering why do you think that the kmalloc-ed buffer is more
> > preferable?
> 

> Because it clearly documents the intent.  Here is one buffer that is
> just a data buffer, and here is one with kernel internal structure.
> The concept of embedding on-disk / on-the-wire structures into internal
> stuctures always seemed rather weird and unexpected to me, as we now
> need to ensure that the alignment works right on both sides.  With
> the right annotations (as done in this series) this will work, but
> it feels a little fragile to me.

IMO both the approaches seem unclear if a reader doesn't know what
they have been introduced for. Anyway do you insist on using the
kmalloc-ed buffer here instead? If so I'll resubmit the series with
this patch updated accordingly.

-Sergey

> 
> > What would be the best solution if we had a qualifier like this:
> > #ifdef CONFIG_DMA_NONCOHERENT
> > #define ____dma_buffer ____cacheline_aligned
> > #else
> > #define ____dma_buffer
> > #endif
> > and used it instead of the direct ____cacheline_aligned utilization.
> 
> So independent of my preference for separate allocations, this suggested
> additional would still be very useful for the places where we need
> to use the alignment for performance or other reasons.  I'd use
> something like __dma_alligned or similar, though.

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 2/2] block: sed-opal: Cache-line-align the cmd/resp buffers
  2022-09-11 16:28     ` Serge Semin
@ 2022-09-25 22:30       ` Serge Semin
  0 siblings, 0 replies; 17+ messages in thread
From: Serge Semin @ 2022-09-25 22:30 UTC (permalink / raw)
  To: Christoph Hellwig, Jonathan Derrick, Revanth Rajashekar, Jens Axboe
  Cc: Serge Semin, Jonathan Derrick, Revanth Rajashekar, Jens Axboe,
	Keith Busch, Jens Axboe, Sagi Grimberg, Rafael Antognolli,
	Scott Bauer, Alexey Malahov, Pavel Parkhomenko,
	Thomas Bogendoerfer, linux-nvme, linux-block, linux-kernel

@Jens, @Revanth, @Jonathan do you have anything to say regarding the
patch and what @Christoph suggested?

On Sun, Sep 11, 2022 at 07:28:57PM +0300, Serge Semin wrote:
> Hello Christoph
> 
> On Sat, Sep 10, 2022 at 07:32:03AM +0200, Christoph Hellwig wrote:
> > On Fri, Sep 09, 2022 at 10:19:16PM +0300, Serge Semin wrote:
> > > In accordance with [1] the DMA-able memory buffers must be
> > > cacheline-aligned otherwise the cache writing-back and invalidation
> > > performed during the mapping may cause the adjacent data being lost. It's
> > > specifically required for the DMA-noncoherent platforms. Seeing the
> > > opal_dev.{cmd,resp} buffers are used for DMAs in the NVME and SCSI/SD
> > > drivers in framework of the nvme_sec_submit() and sd_sec_submit() methods
> > > respectively we must make sure the passed buffers are cacheline-aligned to
> > > prevent the denoted problem.
> >
>  
> > Same comment as for the previous one, this should work, but I think
> > separate allocations for the DMAable buffers would document the intent
> > much better.  Given that the opal initialization isn't a fast path
> > I don't think that the overhead should matter either.
> 
> Thanks for the comment. I see your point. Let's hear the subsystem
> maintainers out for their opinion regarding the most suitable solution
> in this case. If they get to agree with you I'll resend the series
> with altered fixes.
> 
> -Sergey

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 1/2] nvme-hwmon: Cache-line-align the NVME SMART log-buffer
  2022-09-25 22:23         ` Serge Semin
@ 2022-09-26 14:39           ` Christoph Hellwig
  2022-09-26 19:04             ` Serge Semin
  0 siblings, 1 reply; 17+ messages in thread
From: Christoph Hellwig @ 2022-09-26 14:39 UTC (permalink / raw)
  To: Serge Semin
  Cc: Christoph Hellwig, Serge Semin, Jonathan Derrick,
	Revanth Rajashekar, Jens Axboe, Keith Busch, Jens Axboe,
	Sagi Grimberg, Guenter Roeck, Alexey Malahov, Pavel Parkhomenko,
	Thomas Bogendoerfer, linux-nvme, linux-block, linux-kernel

On Mon, Sep 26, 2022 at 01:23:25AM +0300, Serge Semin wrote:
> IMO both the approaches seem unclear if a reader doesn't know what
> they have been introduced for. Anyway do you insist on using the
> kmalloc-ed buffer here instead? If so I'll resubmit the series with
> this patch updated accordingly.

I don't like the __aligend version too much, but I can live with it if
you strongly prefer it.

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 1/2] nvme-hwmon: Cache-line-align the NVME SMART log-buffer
  2022-09-26 14:39           ` Christoph Hellwig
@ 2022-09-26 19:04             ` Serge Semin
  0 siblings, 0 replies; 17+ messages in thread
From: Serge Semin @ 2022-09-26 19:04 UTC (permalink / raw)
  To: Christoph Hellwig
  Cc: Serge Semin, Jonathan Derrick, Revanth Rajashekar, Jens Axboe,
	Keith Busch, Jens Axboe, Sagi Grimberg, Guenter Roeck,
	Alexey Malahov, Pavel Parkhomenko, Thomas Bogendoerfer,
	linux-nvme, linux-block, linux-kernel

On Mon, Sep 26, 2022 at 04:39:59PM +0200, Christoph Hellwig wrote:
> On Mon, Sep 26, 2022 at 01:23:25AM +0300, Serge Semin wrote:
> > IMO both the approaches seem unclear if a reader doesn't know what
> > they have been introduced for. Anyway do you insist on using the
> > kmalloc-ed buffer here instead? If so I'll resubmit the series with
> > this patch updated accordingly.
> 

> I don't like the __aligend version too much, but I can live with it if
> you strongly prefer it.

What about converting the NVME hwmon driver to using the kmalloc'ed
buffer (seeing the rest of the NVME core driver prefer kmallocing the
buffers) and adding the ____cacheline_aligned modifier to the opal_dev
buffers?

-Sergey

^ permalink raw reply	[flat|nested] 17+ messages in thread

end of thread, other threads:[~2022-09-26 19:05 UTC | newest]

Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-09-09 19:19 [PATCH 0/2] block/nvme: Fix DMA-noncoherent platforms support Serge Semin
2022-09-09 19:19 ` [PATCH 1/2] nvme-hwmon: Cache-line-align the NVME SMART log-buffer Serge Semin
2022-09-09 19:42   ` Keith Busch
2022-09-09 20:53     ` Serge Semin
2022-09-09 20:36   ` Guenter Roeck
2022-09-10  5:30   ` Christoph Hellwig
2022-09-10 12:35     ` Serge Semin
2022-09-10 18:09       ` Serge Semin
2022-09-12  8:29       ` Christoph Hellwig
2022-09-25 22:23         ` Serge Semin
2022-09-26 14:39           ` Christoph Hellwig
2022-09-26 19:04             ` Serge Semin
2022-09-10 14:33     ` Guenter Roeck
2022-09-09 19:19 ` [PATCH 2/2] block: sed-opal: Cache-line-align the cmd/resp buffers Serge Semin
2022-09-10  5:32   ` Christoph Hellwig
2022-09-11 16:28     ` Serge Semin
2022-09-25 22:30       ` Serge Semin

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).