From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S938608AbcHJUic (ORCPT ); Wed, 10 Aug 2016 16:38:32 -0400 Received: from mga03.intel.com ([134.134.136.65]:22000 "EHLO mga03.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753125AbcHJSPm (ORCPT ); Wed, 10 Aug 2016 14:15:42 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.28,499,1464678000"; d="scan'208";a="153561738" Date: Wed, 10 Aug 2016 13:36:45 +0300 From: Jarkko Sakkinen To: Dmitry Torokhov Cc: Christophe Ricard , "linux-kernel@vger.kernel.org" , tpmdd-devel@lists.sourceforge.net Subject: Re: [tpmdd-devel] [PATCH] tpm: fix cacheline alignment for DMA-able buffers Message-ID: <20160810103645.GA12832@intel.com> References: <1469761153-85576-1-git-send-email-apronin@chromium.org> <20160729172702.GB7020@obsidianresearch.com> <20160809094610.GA13566@intel.com> <20160809150114.GA9672@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: Organization: Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo User-Agent: Mutt/1.5.24 (2015-08-30) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, Aug 09, 2016 at 08:18:00AM -0700, Dmitry Torokhov wrote: > On Tue, Aug 9, 2016 at 8:01 AM, Jarkko Sakkinen > 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 > > >    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 > > >      >  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