[PATCH] tpm: fix cacheline alignment for DMA-able buffers

[PATCH] tpm: fix cacheline alignment for DMA-able buffers

[Andrey Pronin]

Annotate buffers used in spi transactions as ____cacheline_aligned
to use in DMA transfers.

Signed-off-by: Andrey Pronin <apronin@chromium.org>
---
 drivers/char/tpm/st33zp24/spi.c | 4 ++--
 drivers/char/tpm/tpm_tis_spi.c  | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/char/tpm/st33zp24/spi.c b/drivers/char/tpm/st33zp24/spi.c
index 9f5a011..0e9aad9 100644
--- a/drivers/char/tpm/st33zp24/spi.c
+++ b/drivers/char/tpm/st33zp24/spi.c
@@ -70,8 +70,8 @@
 struct st33zp24_spi_phy {
 	struct spi_device *spi_device;
 
-	u8 tx_buf[ST33ZP24_SPI_BUFFER_SIZE];
-	u8 rx_buf[ST33ZP24_SPI_BUFFER_SIZE];
+	u8 tx_buf[ST33ZP24_SPI_BUFFER_SIZE] ____cacheline_aligned;
+	u8 rx_buf[ST33ZP24_SPI_BUFFER_SIZE] ____cacheline_aligned;
 
 	int io_lpcpd;
 	int latency;
diff --git a/drivers/char/tpm/tpm_tis_spi.c b/drivers/char/tpm/tpm_tis_spi.c
index dbaad9c..58d7758 100644
--- a/drivers/char/tpm/tpm_tis_spi.c
+++ b/drivers/char/tpm/tpm_tis_spi.c
@@ -48,8 +48,8 @@ struct tpm_tis_spi_phy {
 	struct tpm_tis_data priv;
 	struct spi_device *spi_device;
 
-	u8 tx_buf[MAX_SPI_FRAMESIZE + 4];
-	u8 rx_buf[MAX_SPI_FRAMESIZE + 4];
+	u8 tx_buf[MAX_SPI_FRAMESIZE + 4] ____cacheline_aligned;
+	u8 rx_buf[MAX_SPI_FRAMESIZE + 4] ____cacheline_aligned;
 };
 
 static inline struct tpm_tis_spi_phy *to_tpm_tis_spi_phy(struct tpm_tis_data *data)
-- 
2.6.6

Re: [PATCH] tpm: fix cacheline alignment for DMA-able buffers

[Jason Gunthorpe]

On Thu, Jul 28, 2016 at 07:59:13PM -0700, Andrey Pronin wrote:
> Annotate buffers used in spi transactions as ____cacheline_aligned
> to use in DMA transfers.
> 
> Signed-off-by: Andrey Pronin <apronin@chromium.org>
>  drivers/char/tpm/st33zp24/spi.c | 4 ++--
>  drivers/char/tpm/tpm_tis_spi.c  | 4 ++--
>  2 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/char/tpm/st33zp24/spi.c b/drivers/char/tpm/st33zp24/spi.c
> index 9f5a011..0e9aad9 100644
> +++ b/drivers/char/tpm/st33zp24/spi.c
> @@ -70,8 +70,8 @@
>  struct st33zp24_spi_phy {
>  	struct spi_device *spi_device;
>  
> -	u8 tx_buf[ST33ZP24_SPI_BUFFER_SIZE];
> -	u8 rx_buf[ST33ZP24_SPI_BUFFER_SIZE];
> +	u8 tx_buf[ST33ZP24_SPI_BUFFER_SIZE] ____cacheline_aligned;
> +	u8 rx_buf[ST33ZP24_SPI_BUFFER_SIZE] ____cacheline_aligned;
>  
>  	int io_lpcpd;
>  	int latency;

Hurm, this still looks wrong to me. Aligning the start of buffers is
not enough, the DMA'able space must also end on a cache line as well.

So, the buffers must also always be placed at the end of the struct.

IMHO It would be cleaner and safer to always kmalloc the DMA buffer
alone than to try and optimize like this.

Jason

Re: [PATCH] tpm: fix cacheline alignment for DMA-able buffers

[Jarkko Sakkinen]

On Fri, Jul 29, 2016 at 10:30:22AM -0700, Dmitry Torokhov wrote:
>    On Fri, Jul 29, 2016 at 10:27 AM, Jason Gunthorpe
>    <jgunthorpe@obsidianresearch.com> wrote:
> 
>      On Thu, Jul 28, 2016 at 07:59:13PM -0700, Andrey Pronin wrote:
>      > Annotate buffers used in spi transactions as ____cacheline_aligned
>      > to use in DMA transfers.
>      >
>      > Signed-off-by: Andrey Pronin <apronin@chromium.org>
>      >  drivers/char/tpm/st33zp24/spi.c | 4 ++--
>      >  drivers/char/tpm/tpm_tis_spi.c  | 4 ++--
>      >  2 files changed, 4 insertions(+), 4 deletions(-)
>      >
>      > diff --git a/drivers/char/tpm/st33zp24/spi.c
>      b/drivers/char/tpm/st33zp24/spi.c
>      > index 9f5a011..0e9aad9 100644
>      > +++ b/drivers/char/tpm/st33zp24/spi.c
>      > @@ -70,8 +70,8 @@
>      >  struct st33zp24_spi_phy {
>      >       struct spi_device *spi_device;
>      >
>      > -     u8 tx_buf[ST33ZP24_SPI_BUFFER_SIZE];
>      > -     u8 rx_buf[ST33ZP24_SPI_BUFFER_SIZE];
>      > +     u8 tx_buf[ST33ZP24_SPI_BUFFER_SIZE] ____cacheline_aligned;
>      > +     u8 rx_buf[ST33ZP24_SPI_BUFFER_SIZE] ____cacheline_aligned;
>      >
>      >       int io_lpcpd;
>      >       int latency;
> 
>      Hurm, this still looks wrong to me. Aligning the start of buffers is
>      not enough, the DMA'able space must also end on a cache line as well.
> 
>      So, the buffers must also always be placed at the end of the struct.
> 
>      IMHO It would be cleaner and safer to always kmalloc the DMA buffer
>      alone than to try and optimize like this.
> 
>    In this case moving them to the end of the structure and commenting why
>    they have to be at the end might be less invasive change. More
>    performance-efficient and resilient in low memory situations too.

kmallocs would be done in the driver initialization:

* you rarely are in low memory situation
* performance gain/loss is insignificant

I really don't see your point.

>    Thanks,
>    Dmitry

/Jarkko

Re: [tpmdd-devel] [PATCH] tpm: fix cacheline alignment for DMA-able buffers

[Jarkko Sakkinen]

On Tue, Aug 09, 2016 at 12:46:10PM +0300, Jarkko Sakkinen wrote:
> On Fri, Jul 29, 2016 at 10:30:22AM -0700, Dmitry Torokhov wrote:
> >    On Fri, Jul 29, 2016 at 10:27 AM, Jason Gunthorpe
> >    <jgunthorpe@obsidianresearch.com> wrote:
> > 
> >      On Thu, Jul 28, 2016 at 07:59:13PM -0700, Andrey Pronin wrote:
> >      > Annotate buffers used in spi transactions as ____cacheline_aligned
> >      > to use in DMA transfers.
> >      >
> >      > Signed-off-by: Andrey Pronin <apronin@chromium.org>
> >      >  drivers/char/tpm/st33zp24/spi.c | 4 ++--
> >      >  drivers/char/tpm/tpm_tis_spi.c  | 4 ++--
> >      >  2 files changed, 4 insertions(+), 4 deletions(-)
> >      >
> >      > diff --git a/drivers/char/tpm/st33zp24/spi.c
> >      b/drivers/char/tpm/st33zp24/spi.c
> >      > index 9f5a011..0e9aad9 100644
> >      > +++ b/drivers/char/tpm/st33zp24/spi.c
> >      > @@ -70,8 +70,8 @@
> >      >  struct st33zp24_spi_phy {
> >      >       struct spi_device *spi_device;
> >      >
> >      > -     u8 tx_buf[ST33ZP24_SPI_BUFFER_SIZE];
> >      > -     u8 rx_buf[ST33ZP24_SPI_BUFFER_SIZE];
> >      > +     u8 tx_buf[ST33ZP24_SPI_BUFFER_SIZE] ____cacheline_aligned;
> >      > +     u8 rx_buf[ST33ZP24_SPI_BUFFER_SIZE] ____cacheline_aligned;
> >      >
> >      >       int io_lpcpd;
> >      >       int latency;
> > 
> >      Hurm, this still looks wrong to me. Aligning the start of buffers is
> >      not enough, the DMA'able space must also end on a cache line as well.
> > 
> >      So, the buffers must also always be placed at the end of the struct.
> > 
> >      IMHO It would be cleaner and safer to always kmalloc the DMA buffer
> >      alone than to try and optimize like this.
> > 
> >    In this case moving them to the end of the structure and commenting why
> >    they have to be at the end might be less invasive change. More
> >    performance-efficient and resilient in low memory situations too.
> 
> kmallocs would be done in the driver initialization:
> 
> * you rarely are in low memory situation
> * performance gain/loss is insignificant
> 
> I really don't see your point.

I'm fine having them at the end of the structure mainly for simplicity
reasons but those arguments just didn't hold at all.

/Jarkko

Re: [tpmdd-devel] [PATCH] tpm: fix cacheline alignment for DMA-able buffers

[Jason Gunthorpe]

On Tue, Aug 09, 2016 at 08:18:00AM -0700, Dmitry Torokhov wrote:

>    Well, the main reason was simplicity and invasiveness of the
>    change.

Well, it isn't simple, because the proposed patches have had subtle
problems with DMA. Simple is to use a guaranteed dma-able allocation
for DMA memory and stop trying to over optimize.

Jason

Re: [tpmdd-devel] [PATCH] tpm: fix cacheline alignment for DMA-able buffers

[Jarkko Sakkinen]

On Tue, Aug 09, 2016 at 08:18:00AM -0700, Dmitry Torokhov wrote:
>    On Tue, Aug 9, 2016 at 8:01 AM, Jarkko Sakkinen
>    <jarkko.sakkinen@linux.intel.com> wrote:
> 
>      On Tue, Aug 09, 2016 at 12:46:10PM +0300, Jarkko Sakkinen wrote:
>      > On Fri, Jul 29, 2016 at 10:30:22AM -0700, Dmitry Torokhov wrote:
>      > >    On Fri, Jul 29, 2016 at 10:27 AM, Jason Gunthorpe
>      > >    <jgunthorpe@obsidianresearch.com> wrote:
>      > >
>      > >      On Thu, Jul 28, 2016 at 07:59:13PM -0700, Andrey Pronin
>      wrote:
>      > >      > Annotate buffers used in spi transactions as
>      ____cacheline_aligned
>      > >      > to use in DMA transfers.
>      > >      >
>      > >      > Signed-off-by: Andrey Pronin <apronin@chromium.org>
>      > >      >  drivers/char/tpm/st33zp24/spi.c | 4 ++--
>      > >      >  drivers/char/tpm/tpm_tis_spi.c  | 4 ++--
>      > >      >  2 files changed, 4 insertions(+), 4 deletions(-)
>      > >      >
>      > >      > diff --git a/drivers/char/tpm/st33zp24/spi.c
>      > >      b/drivers/char/tpm/st33zp24/spi.c
>      > >      > index 9f5a011..0e9aad9 100644
>      > >      > +++ b/drivers/char/tpm/st33zp24/spi.c
>      > >      > @@ -70,8 +70,8 @@
>      > >      >  struct st33zp24_spi_phy {
>      > >      >       struct spi_device *spi_device;
>      > >      >
>      > >      > -     u8 tx_buf[ST33ZP24_SPI_BUFFER_SIZE];
>      > >      > -     u8 rx_buf[ST33ZP24_SPI_BUFFER_SIZE];
>      > >      > +     u8 tx_buf[ST33ZP24_SPI_BUFFER_SIZE]
>      ____cacheline_aligned;
>      > >      > +     u8 rx_buf[ST33ZP24_SPI_BUFFER_SIZE]
>      ____cacheline_aligned;
>      > >      >
>      > >      >       int io_lpcpd;
>      > >      >       int latency;
>      > >
>      > >      Hurm, this still looks wrong to me. Aligning the start of
>      buffers is
>      > >      not enough, the DMA'able space must also end on a cache line
>      as well.
>      > >
>      > >      So, the buffers must also always be placed at the end of the
>      struct.
>      > >
>      > >      IMHO It would be cleaner and safer to always kmalloc the DMA
>      buffer
>      > >      alone than to try and optimize like this.
>      > >
>      > >    In this case moving them to the end of the structure and
>      commenting why
>      > >    they have to be at the end might be less invasive change. More
>      > >    performance-efficient and resilient in low memory situations
>      too.
>      >
>      > kmallocs would be done in the driver initialization:
>      >
>      > * you rarely are in low memory situation
>      > * performance gain/loss is insignificant
>      >
>      > I really don't see your point.
> 
>      I'm fine having them at the end of the structure mainly for simplicity
>      reasons but those arguments just didn't hold at all.
> 
>    Well, the main reason was simplicity and invasiveness of the change.
>    But I still maintain that doing 3 memory allocations instead of 1 is less
>    performant and puts more pressure on the kernel. Yes, it is at bind time,
>    but you do not have to do 3 times work when one allocation will suffice.
>    Also, driver binding does not necessarily happen at boot time. I can
>    always unbind and rebind the driver or reload the module.

I'm fine with either approach.

>    Thanks,
>    Dmitry

/Jarkko