libata/sata_sil24 cache alignment problem?

libata/sata_sil24 cache alignment problem?

[Mark Mason]

Hi All,

I've implemented the Linux PCI support for a new architecture, and
have run into what appears to be a bug in libata, but I don't
understand why it wouldn't have been seen on other architectures.

The processor is a Tile64, which is a 64 core, 64 bit VLIW processor
with non-coherent DMA - DMA transfers bypass the cache, so cache
coherence must be maintained manually.

I am working with libata v2.0, from Linux 2.6.18, and the sata_sil24
driver for a Silicon Images SIL3531A chip.

The problem occurs when the drive's model and serial numbers are read
at startup via ata_dev_read_id().  They are read once on driver
startup, then when the the error handler first runs it verifies that
the model and serial numbers match what was read when the driver
started.

The problem is with the proximity of the private_data and sector_buf
members of the ata_port struct, and the sector_buf not being cache
aligned.  ata_qc_issue() calls dma_map_single(), which in my case
flushes and invalidates the cache for the buffer it's about DMA into
(ata_port->sector_buf), then calls sil24_qc_issue().

sil24_qc_issue() then dereferences ata_port->private_data, which
causes the cache line containing it to be read in, bringing part of
the sector_buf with it, before the DMA transfer has taken place.  The
Tile64 processor does not have a cache invalidate instruction, only a
flush/invalidate, so dma_unmap_single() can't invalidate the portion
of the sector_buf that's been inadvertently read in, and I get a
serial number and/or model number mismatch, due to part of the buffer
being stale.

Documentation/DMA-API.txt states that addresses passed to
dma_map_single() and dma_unmap_single() must be cache aligned for this
reason.  Both calls to ata_dev_read_id() request DMA into a non
cache-aligned buffer, but only the second call - via
ata_dev_same_device() - has a conflict that can cause the cache line
to be read back in during the very narrow window for which this
vulnerability exists.

Has anyone else reported a problem like this?  It requires
non-coherent DMA, and a lack of a cache invalidate instruction, and
one of the drivers that has this problem (it looks like sata_qstor
does too, I haven't looked at others), so maybe that doesn't cover
any other architectures.

I can provide a patch if you're interested.

Re: libata/sata_sil24 cache alignment problem?

[Alan Cox]

> Has anyone else reported a problem like this?  It requires
> non-coherent DMA, and a lack of a cache invalidate instruction, and
> one of the drivers that has this problem (it looks like sata_qstor
> does too, I haven't looked at others), so maybe that doesn't cover
> any other architectures.

Nobody has, not even PA-RISC which is normally guaranteed to make life
miserable in the caching area but I agree entirely with your diagnosis
and that buffer should indeed be marked cache aligned

> I can provide a patch if you're interested.

Please do.

Alan

Re: libata/sata_sil24 cache alignment problem?

[Thomas Evans]

I wonder if this may be what I am seeing with the Si3124 on my Alpha 
based setup.

I'm not sure if Alpha meets all the criteria, but the thing refuses to 
recognize any drivers when they are connected and I see what are 
supposed pci parity failures.

maybe not ...

...tom


Mark Mason wrote:
> Hi All,
>
> I've implemented the Linux PCI support for a new architecture, and
> have run into what appears to be a bug in libata, but I don't
> understand why it wouldn't have been seen on other architectures.
>
> The processor is a Tile64, which is a 64 core, 64 bit VLIW processor
> with non-coherent DMA - DMA transfers bypass the cache, so cache
> coherence must be maintained manually.
>
> I am working with libata v2.0, from Linux 2.6.18, and the sata_sil24
> driver for a Silicon Images SIL3531A chip.
>
> The problem occurs when the drive's model and serial numbers are read
> at startup via ata_dev_read_id().  They are read once on driver
> startup, then when the the error handler first runs it verifies that
> the model and serial numbers match what was read when the driver
> started.
>
> The problem is with the proximity of the private_data and sector_buf
> members of the ata_port struct, and the sector_buf not being cache
> aligned.  ata_qc_issue() calls dma_map_single(), which in my case
> flushes and invalidates the cache for the buffer it's about DMA into
> (ata_port->sector_buf), then calls sil24_qc_issue().
>
> sil24_qc_issue() then dereferences ata_port->private_data, which
> causes the cache line containing it to be read in, bringing part of
> the sector_buf with it, before the DMA transfer has taken place.  The
> Tile64 processor does not have a cache invalidate instruction, only a
> flush/invalidate, so dma_unmap_single() can't invalidate the portion
> of the sector_buf that's been inadvertently read in, and I get a
> serial number and/or model number mismatch, due to part of the buffer
> being stale.
>
> Documentation/DMA-API.txt states that addresses passed to
> dma_map_single() and dma_unmap_single() must be cache aligned for this
> reason.  Both calls to ata_dev_read_id() request DMA into a non
> cache-aligned buffer, but only the second call - via
> ata_dev_same_device() - has a conflict that can cause the cache line
> to be read back in during the very narrow window for which this
> vulnerability exists.
>
> Has anyone else reported a problem like this?  It requires
> non-coherent DMA, and a lack of a cache invalidate instruction, and
> one of the drivers that has this problem (it looks like sata_qstor
> does too, I haven't looked at others), so maybe that doesn't cover
> any other architectures.
>
> I can provide a patch if you're interested.
> -
> To unsubscribe from this list: send the line "unsubscribe linux-ide" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>   

Re: libata/sata_sil24 cache alignment problem?

[James Bottomley]

On Tue, 2008-02-12 at 22:12 +0000, Alan Cox wrote:
> > Has anyone else reported a problem like this?  It requires
> > non-coherent DMA, and a lack of a cache invalidate instruction, and
> > one of the drivers that has this problem (it looks like sata_qstor
> > does too, I haven't looked at others), so maybe that doesn't cover
> > any other architectures.
> 
> Nobody has, not even PA-RISC which is normally guaranteed to make life
> miserable in the caching area but I agree entirely with your diagnosis
> and that buffer should indeed be marked cache aligned

I tell you what ... find me a parisc box that actually has IDE and we
might have told you ...

(actually, the pa8800's have IDE CD's on a cmd640 chip, but that oopses
on boot for no reason we've tracked yet).

James

Re: libata/sata_sil24 cache alignment problem?

[Alan Cox]

> I tell you what ... find me a parisc box that actually has IDE and we
> might have told you ...

The NS87415 variant IDE has been tested on parisc and didn't blow up -
must just be lucky.

> (actually, the pa8800's have IDE CD's on a cmd640 chip, but that oopses
> on boot for no reason we've tracked yet).

With libata, old IDE or both ? and what does the oops look like ?

Re: libata/sata_sil24 cache alignment problem?

[James Bottomley]

On Wed, 2008-02-13 at 02:13 +0000, Alan Cox wrote:
> > I tell you what ... find me a parisc box that actually has IDE and we
> > might have told you ...
> 
> The NS87415 variant IDE has been tested on parisc and didn't blow up -
> must just be lucky.

Actually, it's only a specific class of machines: the PCX (that's most
of the 715,725 series) that are fully incoherent.  PCXL hack around this
by making the page uncacheable and the 64 bit machines have coherence
index handling which fixes this up (mostly).

> > (actually, the pa8800's have IDE CD's on a cmd640 chip, but that oopses
> > on boot for no reason we've tracked yet).
> 
> With libata, old IDE or both ? and what does the oops look like ?

Unfortunately, there's no oops; we get a high priority machine check
which means something has gone wrong somewhere and it's not pretty.
It's on my todo list to track down ... I just don't have one of these
boxes and CDs are hard to play with remotely.

James

Re: libata/sata_sil24 cache alignment problem?

[Mark Mason]

Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:

> > Has anyone else reported a problem like this?  It requires
> > non-coherent DMA, and a lack of a cache invalidate instruction, and
> > one of the drivers that has this problem (it looks like sata_qstor
> > does too, I haven't looked at others), so maybe that doesn't cover
> > any other architectures.
> 
> Nobody has, not even PA-RISC which is normally guaranteed to make life
> miserable in the caching area but I agree entirely with your diagnosis
> and that buffer should indeed be marked cache aligned
> 
> > I can provide a patch if you're interested.
> 
> Please do.

Here it is.  I'd appreciate it if someone could sanity check where I'm
freeing the buffer.  It works, but I don't know the code well enough
to know if this covers all cases.

I'm counting on kmalloc to return a cache aligned buffer.  I found
some reason to think it does, but I don't remember offhand what that
reason was, or if it's configurable per-architecture.  The buffer has
to be both physically and virtually contiguous, I was tempted to just
allocate a page and waste some space but we've got 64K pages, so I'm a
bit more sensitive about that.



*** drivers/scsi/libata-core.c	2008-02-13 13:34:32.141489000 -0500
--- drivers/scsi/libata-core.c.orig	2008-02-13 13:29:35.620360000 -0500
***************
*** 5335,5369 ****
  	}
  
  	host = scsi_host_alloc(ent->sht, sizeof(struct ata_port));
  	if (!host)
  		return NULL;
  
  	host->transportt = &ata_scsi_transport_template;
  
  	ap = ata_shost_to_port(host);
  
- 	ap->sector_buf = kmalloc(ATA_SECT_SIZE, GFP_KERNEL);
- 	if (!ap->sector_buf)
- 		goto err_out;
- 
  	ata_host_init(ap, host, host_set, ent, port_no);
  
  	rc = ap->ops->port_start(ap);
  	if (rc)
  		goto err_out;
  
  	return ap;
  
  err_out:
- 	if (ap->sector_buf)
- 		kfree(ap->sector_buf);
  	scsi_host_put(host);
  	return NULL;
  }
  
  /**
   *	ata_device_add - Register hardware device with ATA and SCSI layers
   *	@ent: Probe information describing hardware device to be registered
   *
   *	This function processes the information provided in the probe
   *	information struct @ent, allocates the necessary ATA and SCSI
--- 5335,5363 ----
***************
*** 5602,5645 ****
   *	Unregister all objects associated with this host set. Free those
   *	objects.
   *
   *	LOCKING:
   *	Inherited from calling layer (may sleep).
   */
  
  void ata_host_set_remove(struct ata_host_set *host_set)
  {
  	unsigned int i;
- 	u8 *sector_buf;
  
  	for (i = 0; i < host_set->n_ports; i++)
  		ata_port_detach(host_set->ports[i]);
  
  	free_irq(host_set->irq, host_set);
  
  	for (i = 0; i < host_set->n_ports; i++) {
  		struct ata_port *ap = host_set->ports[i];
  
  		ata_scsi_release(ap->host);
  
  		if ((ap->flags & ATA_FLAG_NO_LEGACY) == 0) {
  			struct ata_ioports *ioaddr = &ap->ioaddr;
  
  			if (ioaddr->cmd_addr == 0x1f0)
  				release_region(0x1f0, 8);
  			else if (ioaddr->cmd_addr == 0x170)
  				release_region(0x170, 8);
  		}
  
- 		sector_buf = ap->sector_buf;
  		scsi_host_put(ap->host);
- 		kfree(sector_buf);
  	}
  
  	if (host_set->ops->host_stop)
  		host_set->ops->host_stop(host_set);
  
  	kfree(host_set);
  }
  
  /**
   *	ata_scsi_release - SCSI layer callback hook for host unload
--- 5596,5636 ----

Re: libata/sata_sil24 cache alignment problem?

[Mark Mason]

Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:

> > Has anyone else reported a problem like this?  It requires
> > non-coherent DMA, and a lack of a cache invalidate instruction, and
> > one of the drivers that has this problem (it looks like sata_qstor
> > does too, I haven't looked at others), so maybe that doesn't cover
> > any other architectures.
> 
> Nobody has, not even PA-RISC which is normally guaranteed to make life
> miserable in the caching area but I agree entirely with your diagnosis
> and that buffer should indeed be marked cache aligned
> 
> > I can provide a patch if you're interested.
> 
> Please do.

Oops, sorry, I left a file out of that diff.  Here it is again...

*** drivers/scsi/libata-core.c	2008-02-13 13:34:32.141489000 -0500
--- drivers/scsi/libata-core.c.orig	2008-02-13 13:29:35.620360000 -0500
***************
*** 5335,5369 ****
  	}
  
  	host = scsi_host_alloc(ent->sht, sizeof(struct ata_port));
  	if (!host)
  		return NULL;
  
  	host->transportt = &ata_scsi_transport_template;
  
  	ap = ata_shost_to_port(host);
  
- 	ap->sector_buf = kmalloc(ATA_SECT_SIZE, GFP_KERNEL);
- 	if (!ap->sector_buf)
- 		goto err_out;
- 
  	ata_host_init(ap, host, host_set, ent, port_no);
  
  	rc = ap->ops->port_start(ap);
  	if (rc)
  		goto err_out;
  
  	return ap;
  
  err_out:
- 	if (ap->sector_buf)
- 		kfree(ap->sector_buf);
  	scsi_host_put(host);
  	return NULL;
  }
  
  /**
   *	ata_device_add - Register hardware device with ATA and SCSI layers
   *	@ent: Probe information describing hardware device to be registered
   *
   *	This function processes the information provided in the probe
   *	information struct @ent, allocates the necessary ATA and SCSI
--- 5335,5363 ----
***************
*** 5602,5645 ****
   *	Unregister all objects associated with this host set. Free those
   *	objects.
   *
   *	LOCKING:
   *	Inherited from calling layer (may sleep).
   */
  
  void ata_host_set_remove(struct ata_host_set *host_set)
  {
  	unsigned int i;
- 	u8 *sector_buf;
  
  	for (i = 0; i < host_set->n_ports; i++)
  		ata_port_detach(host_set->ports[i]);
  
  	free_irq(host_set->irq, host_set);
  
  	for (i = 0; i < host_set->n_ports; i++) {
  		struct ata_port *ap = host_set->ports[i];
  
  		ata_scsi_release(ap->host);
  
  		if ((ap->flags & ATA_FLAG_NO_LEGACY) == 0) {
  			struct ata_ioports *ioaddr = &ap->ioaddr;
  
  			if (ioaddr->cmd_addr == 0x1f0)
  				release_region(0x1f0, 8);
  			else if (ioaddr->cmd_addr == 0x170)
  				release_region(0x170, 8);
  		}
  
- 		sector_buf = ap->sector_buf;
  		scsi_host_put(ap->host);
- 		kfree(sector_buf);
  	}
  
  	if (host_set->ops->host_stop)
  		host_set->ops->host_stop(host_set);
  
  	kfree(host_set);
  }
  
  /**
   *	ata_scsi_release - SCSI layer callback hook for host unload
--- 5596,5636 ----
*** include/linux/libata.h	2008-02-13 10:51:39.151202000 -0500
--- include/linux/libata.h.orig	2008-02-13 13:50:26.531572000 -0500
***************
*** 550,570 ****
  
  	u32			msg_enable;
  	struct list_head	eh_done_q;
  	wait_queue_head_t	eh_wait_q;
  
  	pm_message_t		pm_mesg;
  	int			*pm_result;
  
  	void			*private_data;
  
! 	u8			*sector_buf; /* owned by EH */
  };
  
  struct ata_port_operations {
  	void (*port_disable) (struct ata_port *);
  
  	void (*dev_config) (struct ata_port *, struct ata_device *);
  
  	void (*set_piomode) (struct ata_port *, struct ata_device *);
  	void (*set_dmamode) (struct ata_port *, struct ata_device *);
  	unsigned long (*mode_filter) (const struct ata_port *, struct ata_device *, unsigned long);
--- 550,570 ----
  
  	u32			msg_enable;
  	struct list_head	eh_done_q;
  	wait_queue_head_t	eh_wait_q;
  
  	pm_message_t		pm_mesg;
  	int			*pm_result;
  
  	void			*private_data;
  
! 	u8			sector_buf[ATA_SECT_SIZE]; /* owned by EH */
  };
  
  struct ata_port_operations {
  	void (*port_disable) (struct ata_port *);
  
  	void (*dev_config) (struct ata_port *, struct ata_device *);
  
  	void (*set_piomode) (struct ata_port *, struct ata_device *);
  	void (*set_dmamode) (struct ata_port *, struct ata_device *);
  	unsigned long (*mode_filter) (const struct ata_port *, struct ata_device *, unsigned long);

Re: libata/sata_sil24 cache alignment problem?

[Alan Cox]

O> I'm counting on kmalloc to return a cache aligned buffer.  I found
> some reason to think it does, but I don't remember offhand what that

Its defined to

> reason was, or if it's configurable per-architecture.  The buffer has
> to be both physically and virtually contiguous, I was tempted to just
> allocate a page and waste some space but we've got 64K pages, so I'm a
> bit more sensitive about that.

Ok I was expecting a different approach if you mark the field with the
magic  ____cacheline_aligned tag after it (ie int foo ____blah_aligned;)
the compiler should align it all for you , which is probably cleaner if
it works.

Re: libata/sata_sil24 cache alignment problem?

[Mark Mason]

Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:

> O> I'm counting on kmalloc to return a cache aligned buffer.  I found
> > some reason to think it does, but I don't remember offhand what that
> 
> Its defined to
> 
> > reason was, or if it's configurable per-architecture.  The buffer has
> > to be both physically and virtually contiguous, I was tempted to just
> > allocate a page and waste some space but we've got 64K pages, so I'm a
> > bit more sensitive about that.
> 
> Ok I was expecting a different approach if you mark the field with the
> magic  ____cacheline_aligned tag after it (ie int foo ____blah_aligned;)
> the compiler should align it all for you , which is probably cleaner if
> it works.

I hadn't considered that approach due to the way the ata_port is allocated:

> libata-core.c:
>         host = scsi_host_alloc(ent->sht, sizeof(struct ata_port));
> 
> hosts.c:
> struct Scsi_Host *scsi_host_alloc(struct scsi_host_template *sht, int privsize)
> {
>         shost = kzalloc(sizeof(struct Scsi_Host) + privsize, gfp_mask);
> }

The ata_port allocation is tacked onto the end of the Scsi_Host
allocation, so the start of ata_port will only be cache aligned if the
end of the Scsi_Host struct is, although that would be easy enough to
fix since it's currently aligned to an unsigned long boundary.

I like that approach better, since it's clearer what the intent is,
and it's easier.  Is there any other way that the ata_port struct
might be used that would invalidate this?  This one issue is the
extent of my knowledge of libata.

Re: libata/sata_sil24 cache alignment problem?

[Alan Cox]

> I hadn't considered that approach due to the way the ata_port is allocated:
> 
> > libata-core.c:
> >         host = scsi_host_alloc(ent->sht, sizeof(struct ata_port));
> > 
> > hosts.c:
> > struct Scsi_Host *scsi_host_alloc(struct scsi_host_template *sht, int privsize)
> > {
> >         shost = kzalloc(sizeof(struct Scsi_Host) + privsize, gfp_mask);
> > }
> 
> The ata_port allocation is tacked onto the end of the Scsi_Host
> allocation, so the start of ata_port will only be cache aligned if the
> end of the Scsi_Host struct is, although that would be easy enough to
> fix since it's currently aligned to an unsigned long boundary.

You are right.

> I like that approach better, since it's clearer what the intent is,
> and it's easier.  Is there any other way that the ata_port struct
> might be used that would invalidate this?  

I can't think of one. The object lifetime is right - the ata_port is
created before the port buffer is used and destroyed after it is finished
(obviously or embedding it in the struct wouldn't work either)

Alan

Re: libata/sata_sil24 cache alignment problem?

[Tejun Heo]

Alan Cox wrote:
>> I hadn't considered that approach due to the way the ata_port is allocated:
>>
>>> libata-core.c:
>>>         host = scsi_host_alloc(ent->sht, sizeof(struct ata_port));
>>>
>>> hosts.c:
>>> struct Scsi_Host *scsi_host_alloc(struct scsi_host_template *sht, int privsize)
>>> {
>>>         shost = kzalloc(sizeof(struct Scsi_Host) + privsize, gfp_mask);
>>> }
>> The ata_port allocation is tacked onto the end of the Scsi_Host
>> allocation, so the start of ata_port will only be cache aligned if the
>> end of the Scsi_Host struct is, although that would be easy enough to
>> fix since it's currently aligned to an unsigned long boundary.
> 
> You are right.

For recent kernels, ata_port is allocated separately so
____cacheline_aligned should be enough.

>> I like that approach better, since it's clearer what the intent is,

But, yeah, allocating separately is probably safer.

>> and it's easier.  Is there any other way that the ata_port struct
>> might be used that would invalidate this?  
> 
> I can't think of one. The object lifetime is right - the ata_port is
> created before the port buffer is used and destroyed after it is finished
> (obviously or embedding it in the struct wouldn't work either)

I'll prep a patch for the current kernel.  Hmmm... This means that any
DMA buffer should be aligned to cacheline.  Block layer DMA alignment
helpers should probably take this into consideration and we better add
warnings to dma map functions.  This only affects DMA-to-memory, right?

Thanks.

-- 
tejun

[PATCH] libata: align ap->sector_buf to cacheline

[Tejun Heo]

ap->sector_buf is used as DMA target and misalignment can cause data
corruption on non-coherent architectures.  This problem is spotted and
initial patch is submitted by Mark Mason.

Signed-off-by: Tejun Heo <htejun@gmail.com>
Cc: Mark Mason <mason@postdiluvian.org>
---
I thought about it more and marking up with ___cacheline_aligned seems
to be the right thing to do.  For older ones where ata_port is
allocated as part of scsi_host, I think what should be done is to
align private area of scsi_host to cacheline.  Will post another patch
for that.

Thanks.

 include/linux/libata.h |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
diff --git a/include/linux/libata.h b/include/linux/libata.h
index bc5a8d0..7604763 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -671,7 +671,8 @@ struct ata_port {
 	acpi_handle		acpi_handle;
 	struct ata_acpi_gtm	__acpi_init_gtm; /* use ata_acpi_init_gtm() */
 #endif
-	u8			sector_buf[ATA_SECT_SIZE]; /* owned by EH */
+	/* owned by EH, must be cache line aligned as it's used as DMA target */
+	u8			sector_buf[ATA_SECT_SIZE] ____cacheline_aligned;
 };
 
 struct ata_port_operations {

[PATCH] scsi: align shost->hostdata to cacheline

[Tejun Heo]

shost->hostdata can contain arbitrary data including DMA target
buffers.  Align it to cacheline.

Signed-off-by: Tejun Heo <htejun@gmail.com>
---
James, what do you think?

 include/scsi/scsi_host.h |    8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h
index 5c58d59..3876d24 100644
--- a/include/scsi/scsi_host.h
+++ b/include/scsi/scsi_host.h
@@ -663,12 +663,10 @@ struct Scsi_Host {
 	void *shost_data;
 
 	/*
-	 * We should ensure that this is aligned, both for better performance
-	 * and also because some compilers (m68k) don't automatically force
-	 * alignment to a long boundary.
+	 * Used for storage of host specific stuff.  As it may contain
+	 * arbitrary data, align it to cacheline.
 	 */
-	unsigned long hostdata[0]  /* Used for storage of host specific stuff */
-		__attribute__ ((aligned (sizeof(unsigned long))));
+	unsigned long hostdata[0] ____cacheline_aligned;
 };
 
 #define		class_to_shost(d)	\

Re: [PATCH] scsi: align shost->hostdata to cacheline

[James Bottomley]

On Thu, 2008-02-14 at 18:48 +0900, Tejun Heo wrote:
> shost->hostdata can contain arbitrary data including DMA target
> buffers.  Align it to cacheline.
> 
> Signed-off-by: Tejun Heo <htejun@gmail.com>
> ---
> James, what do you think?

Hmm, it will blow out the host size ... although that's not such a huge
problem since there are relatively few of them in most running kernels.
What's the actual use case for this, though?  The host structure is
allocated in ordinary memory ... we don't make sure it's DMAable, and
most HBAs that want to use memory for mailboxes need coherent memory
anyway.

James

Re: [PATCH] scsi: align shost->hostdata to cacheline

[Tejun Heo]

James Bottomley wrote:
> On Thu, 2008-02-14 at 18:48 +0900, Tejun Heo wrote:
>> shost->hostdata can contain arbitrary data including DMA target
>> buffers.  Align it to cacheline.
>>
>> Signed-off-by: Tejun Heo <htejun@gmail.com>
>> ---
>> James, what do you think?
> 
> Hmm, it will blow out the host size ... although that's not such a huge
> problem since there are relatively few of them in most running kernels.
> What's the actual use case for this, though?  The host structure is
> allocated in ordinary memory ... we don't make sure it's DMAable, and
> most HBAs that want to use memory for mailboxes need coherent memory
> anyway.

"As it can contain arbitrary structure, it should follow the largest
meaningful alignment to allow the contained structure proper alignment."
is the logic.  I think it's generally RTTD for inline private data but
feel free to disagree.

Thanks.

-- 
tejun

Re: [PATCH] scsi: align shost->hostdata to cacheline

[James Bottomley]

On Fri, 2008-02-15 at 07:49 +0900, Tejun Heo wrote:
> James Bottomley wrote:
> > On Thu, 2008-02-14 at 18:48 +0900, Tejun Heo wrote:
> >> shost->hostdata can contain arbitrary data including DMA target
> >> buffers.  Align it to cacheline.
> >>
> >> Signed-off-by: Tejun Heo <htejun@gmail.com>
> >> ---
> >> James, what do you think?
> > 
> > Hmm, it will blow out the host size ... although that's not such a huge
> > problem since there are relatively few of them in most running kernels.
> > What's the actual use case for this, though?  The host structure is
> > allocated in ordinary memory ... we don't make sure it's DMAable, and
> > most HBAs that want to use memory for mailboxes need coherent memory
> > anyway.
> 
> "As it can contain arbitrary structure, it should follow the largest
> meaningful alignment to allow the contained structure proper alignment."
> is the logic.  I think it's generally RTTD for inline private data but
> feel free to disagree.

Well, the way we usually do that is to have the host float the alignment
if necessary.  The problem with relying on __cacheline_aligned for an
allocated structure is that it only works if the allocated structure
actually begins on a cacheline.  Kmalloc (which is where we get the host
from) doesn't necessarily obey this if certain slab debugging flags are
present.

James

Re: [PATCH] scsi: align shost->hostdata to cacheline

[Tejun Heo]

James Bottomley wrote:
> On Fri, 2008-02-15 at 07:49 +0900, Tejun Heo wrote:
>> James Bottomley wrote:
>>> On Thu, 2008-02-14 at 18:48 +0900, Tejun Heo wrote:
>>>> shost->hostdata can contain arbitrary data including DMA target
>>>> buffers.  Align it to cacheline.
>>>>
>>>> Signed-off-by: Tejun Heo <htejun@gmail.com>
>>>> ---
>>>> James, what do you think?
>>> Hmm, it will blow out the host size ... although that's not such a huge
>>> problem since there are relatively few of them in most running kernels.
>>> What's the actual use case for this, though?  The host structure is
>>> allocated in ordinary memory ... we don't make sure it's DMAable, and
>>> most HBAs that want to use memory for mailboxes need coherent memory
>>> anyway.
>> "As it can contain arbitrary structure, it should follow the largest
>> meaningful alignment to allow the contained structure proper alignment."
>> is the logic.  I think it's generally RTTD for inline private data but
>> feel free to disagree.
> 
> Well, the way we usually do that is to have the host float the alignment
> if necessary.  The problem with relying on __cacheline_aligned for an
> allocated structure is that it only works if the allocated structure
> actually begins on a cacheline.  Kmalloc (which is where we get the host
> from) doesn't necessarily obey this if certain slab debugging flags are
> present.

Hmmm... Right.  That means libata either needs to create a separate slab
for the sector buf or align it after allocating by itself.  Eeeek.

Thanks.

-- 
tejun