[patch 1/6] s390, block: disable fixed buffer mode when DMA support is disabled

[patch 1/6] s390, block: disable fixed buffer mode when DMA support is disabled

[David Rientjes]

dasd=fixedbuffers must create a SLAB_CACHE_DMA cache, which is not
possible if CONFIG_ZONE_DMA is disabled (a supported configuration
without 64-bit support).

If passed, emit a warning and disable fixed buffer mode.

Signed-off-by: David Rientjes <rientjes@google.com>
---
 drivers/s390/block/dasd_devmap.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/drivers/s390/block/dasd_devmap.c b/drivers/s390/block/dasd_devmap.c
--- a/drivers/s390/block/dasd_devmap.c
+++ b/drivers/s390/block/dasd_devmap.c
@@ -282,6 +282,11 @@ dasd_parse_keyword( char *parsestring ) {
 		return residual_str;
 	}
 	if (strncmp("fixedbuffers", parsestring, length) == 0) {
+#ifndef CONFIG_ZONE_DMA
+		DBF_EVENT(DBF_WARNING, "%s", "DMA support disabled, "
+					"fixed buffer mode disabled.");
+		return residual_str;
+#endif
 		if (dasd_page_cache)
 			return residual_str;
 		dasd_page_cache =

[patch 2/6] scsi: warn and avoid creating dma caches if DMA support is disabled

[David Rientjes]

__GFP_DMA may not be passed to scsi_{get,put}_host_cmd_pool() if
CONFIG_ZONE_DMA is disabled, which is now possible on x86.  If passed,
emit a warning and return.

Signed-off-by: David Rientjes <rientjes@google.com>
---
 drivers/scsi/scsi.c |    9 +++++++++
 1 files changed, 9 insertions(+), 0 deletions(-)

diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
--- a/drivers/scsi/scsi.c
+++ b/drivers/scsi/scsi.c
@@ -359,6 +359,11 @@ EXPORT_SYMBOL(scsi_put_command);
 static struct scsi_host_cmd_pool *scsi_get_host_cmd_pool(gfp_t gfp_mask)
 {
 	struct scsi_host_cmd_pool *retval = NULL, *pool;
+
+#ifndef CONFIG_ZONE_DMA
+	if (WARN_ON_ONCE(gfp_mask & __GFP_DMA))
+		return NULL;
+#endif
 	/*
 	 * Select a command slab for this host and create it if not
 	 * yet existent.
@@ -393,6 +398,10 @@ static void scsi_put_host_cmd_pool(gfp_t gfp_mask)
 {
 	struct scsi_host_cmd_pool *pool;
 
+#ifndef CONFIG_ZONE_DMA
+        if (WARN_ON_ONCE(gfp_mask & __GFP_DMA))
+                return;
+#endif
 	mutex_lock(&host_cmd_pool_mutex);
 	pool = (gfp_mask & __GFP_DMA) ? &scsi_cmd_dma_pool :
 		&scsi_cmd_pool;

[patch 3/6] scsi, fnic: require DMA support for Cisco FNIC

[David Rientjes]

The Cisco FNIC driver requires creating a SLAB_CACHE_DMA cache, which is
not possible if CONFIG_ZONE_DMA is disasbled.  Avoid compiling it without
DMA support.

Signed-off-by: David Rientjes <rientjes@google.com>
---
 drivers/scsi/Kconfig |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/scsi/Kconfig b/drivers/scsi/Kconfig
--- a/drivers/scsi/Kconfig
+++ b/drivers/scsi/Kconfig
@@ -688,7 +688,7 @@ config FCOE
 
 config FCOE_FNIC
 	tristate "Cisco FNIC Driver"
-	depends on PCI && X86
+	depends on PCI && X86 && ZONE_DMA
 	select LIBFCOE
 	help
 	  This is support for the Cisco PCI-Express FCoE HBA.

Re: [patch 3/6] scsi, fnic: require DMA support for Cisco FNIC

[Christoph Hellwig]

On Tue, May 24, 2011 at 04:53:50PM -0700, David Rientjes wrote:
> The Cisco FNIC driver requires creating a SLAB_CACHE_DMA cache, which is
> not possible if CONFIG_ZONE_DMA is disasbled.  Avoid compiling it without
> DMA support.

And you're sure it actually needs it and isn't some sort of typo?  It
might help to Cc the maintainer to figure that out.

Re: [patch 3/6] scsi, fnic: require DMA support for Cisco FNIC

[Roland Dreier]

On Wed, May 25, 2011 at 1:04 AM, Christoph Hellwig <hch@infradead.org> wrote:
>> The Cisco FNIC driver requires creating a SLAB_CACHE_DMA cache, which is
>> not possible if CONFIG_ZONE_DMA is disasbled.  Avoid compiling it without
>> DMA support.
>
> And you're sure it actually needs it and isn't some sort of typo?  It
> might help to Cc the maintainer to figure that out.

Yes, almost certainly it is due to a misunderstanding of what
SLAB_CACHE_DMA means.
(fnic is a modern PCIe device that doesn't have 24-bit DMA restrictions ;)

So the correct fix would likely be to delete the SLAB_CACHE_DMA from the driver.

In any case cc'ing Cisco people as hch suggested...

 - R.

RE: [patch 3/6] scsi, fnic: require DMA support for Cisco FNIC

[Abhijeet Joglekar (abjoglek)]

> -----Original Message-----
> From: roland@purestorage.com [mailto:roland@purestorage.com] On Behalf
> Of Roland Dreier
> Sent: Wednesday, May 25, 2011 12:29 PM
> To: Christoph Hellwig
> Cc: David Rientjes; James E.J. Bottomley; Pekka Enberg; Christoph
> Lameter; linux-scsi@vger.kernel.org; linux-kernel@vger.kernel.org;
> Abhijeet Joglekar (abjoglek); Joe Eykholt
> Subject: Re: [patch 3/6] scsi, fnic: require DMA support for Cisco FNIC
> 
> On Wed, May 25, 2011 at 1:04 AM, Christoph Hellwig <hch@infradead.org>
> wrote:
> >> The Cisco FNIC driver requires creating a SLAB_CACHE_DMA cache,
> which is
> >> not possible if CONFIG_ZONE_DMA is disasbled.  Avoid compiling it
> without
> >> DMA support.
> >
> > And you're sure it actually needs it and isn't some sort of typo?  It
> > might help to Cc the maintainer to figure that out.
> 
> Yes, almost certainly it is due to a misunderstanding of what
> SLAB_CACHE_DMA means.
> (fnic is a modern PCIe device that doesn't have 24-bit DMA restrictions
> ;)
> 
> So the correct fix would likely be to delete the SLAB_CACHE_DMA from
> the driver.
> 
> In any case cc'ing Cisco people as hch suggested...
> 
>  - R.

You are right, fnic hardware does not have any 24-bit DMA restrictions. When I coded it up, I misunderstood the flag as something that is required for allocating memory that hardware DMAs in/out of.

As Roland indicated, the correct fix would be to remove the SLAB_CACHE_DMA flag from the call to kmem_cache, and the GFP_DMA from the call to the mempool allocation routine.

I can create a patch (attributing the fix to David and Roland) and send out for review. Or if David wants, he can modify the patch he submitted to include this fix. David, please let me know how you want to proceed.

Thanks
-- abhijeet

RE: [patch 3/6] scsi, fnic: require DMA support for Cisco FNIC

[David Rientjes]

On Wed, 25 May 2011, Abhijeet Joglekar (abjoglek) wrote:

> As Roland indicated, the correct fix would be to remove the 
> SLAB_CACHE_DMA flag from the call to kmem_cache, and the GFP_DMA from 
> the call to the mempool allocation routine.
> 
> I can create a patch (attributing the fix to David and Roland) and send 
> out for review. Or if David wants, he can modify the patch he submitted 
> to include this fix. David, please let me know how you want to proceed.
> 

It'd be great if you could create a patch, you would certainly be able to 
speak with more authority on the hardware requirements in the changelog 
than I would :)

Thanks Roland, Christoph, and Abhijeet!

[patch 4/6] slub: avoid compiling SLAB_CACHE_DMA without DMA support

[David Rientjes]

If CONFIG_ZONE_DMA is disabled, SLAB_CACHE_DMA is a no-op.  Avoid
compiling support for it on such a configuration.

Signed-off-by: David Rientjes <rientjes@google.com>
---
 mm/slub.c |    9 ++++++++-
 1 files changed, 8 insertions(+), 1 deletions(-)

diff --git a/mm/slub.c b/mm/slub.c
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -162,7 +162,10 @@ static inline int kmem_cache_debug(struct kmem_cache *s)
 		SLAB_FAILSLAB)
 
 #define SLUB_MERGE_SAME (SLAB_DEBUG_FREE | SLAB_RECLAIM_ACCOUNT | \
-		SLAB_CACHE_DMA | SLAB_NOTRACK)
+# ifdef CONFIG_ZONE_DMA
+		SLAB_CACHE_DMA | \
+# endif
+		SLAB_NOTRACK)
 
 #define OO_SHIFT	16
 #define OO_MASK		((1 << OO_SHIFT) - 1)
@@ -2544,8 +2547,10 @@ static int calculate_sizes(struct kmem_cache *s, int forced_order)
 	if (order)
 		s->allocflags |= __GFP_COMP;
 
+#ifdef CONFIG_ZONE_DMA
 	if (s->flags & SLAB_CACHE_DMA)
 		s->allocflags |= SLUB_DMA;
+#endif
 
 	if (s->flags & SLAB_RECLAIM_ACCOUNT)
 		s->allocflags |= __GFP_RECLAIMABLE;
@@ -4651,8 +4656,10 @@ static char *create_unique_id(struct kmem_cache *s)
 	 * are matched during merging to guarantee that the id is
 	 * unique.
 	 */
+#ifdef CONFIG_ZONE_DMA
 	if (s->flags & SLAB_CACHE_DMA)
 		*p++ = 'd';
+#endif
 	if (s->flags & SLAB_RECLAIM_ACCOUNT)
 		*p++ = 'a';
 	if (s->flags & SLAB_DEBUG_FREE)

Re: [patch 4/6] slub: avoid compiling SLAB_CACHE_DMA without DMA support

[Christoph Lameter]

On Tue, 24 May 2011, David Rientjes wrote:

> If CONFIG_ZONE_DMA is disabled, SLAB_CACHE_DMA is a no-op.  Avoid
> compiling support for it on such a configuration.

Setting SLAB_CACHE_DMA to zero in the !ZONE_DMA case could avoid some of
the ifdeffery.

Re: [patch 4/6] slub: avoid compiling SLAB_CACHE_DMA without DMA support

[David Rientjes]

On Wed, 25 May 2011, Christoph Lameter wrote:

> > If CONFIG_ZONE_DMA is disabled, SLAB_CACHE_DMA is a no-op.  Avoid
> > compiling support for it on such a configuration.
> 
> Setting SLAB_CACHE_DMA to zero in the !ZONE_DMA case could avoid some of
> the ifdeffery.
> 

That doesn't actually accomplish anything since all SLAB_CACHE_DMA does is 
set SLUB_DMA, which is already 0 in slub_def.h without CONFIG_ZONE_DMA:

	#ifdef CONFIG_ZONE_DMA
	#define SLUB_DMA __GFP_DMA  
	#else
	/* Disable DMA functionality */
	#define SLUB_DMA (__force gfp_t)0
	#endif

So doing the same for SLAB_CACHE_DMA wouldn't do anything.  The point of 
the patchset is to break the build if SLAB_CACHE_DMA is passed to 
kmem_cache_create() and the kernel is not compiled for DMA support.  That 
should result in a build breakage rather than silently not supporting DMA 
slab caches since it needs to be corrected at build.

[patch 5/6] slab: avoid compiling SLAB_CACHE_DMA without DMA support

[David Rientjes]

If CONFIG_ZONE_DMA is disabled, SLAB_CACHE_DMA is a no-op.  Avoid
compiling support for it on such a configuration.

Signed-off-by: David Rientjes <rientjes@google.com>
---
 mm/slab.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/mm/slab.c b/mm/slab.c
--- a/mm/slab.c
+++ b/mm/slab.c
@@ -153,14 +153,18 @@
 #if DEBUG
 # define CREATE_MASK	(SLAB_RED_ZONE | \
 			 SLAB_POISON | SLAB_HWCACHE_ALIGN | \
+# ifdef CONFIG_ZONE_DMA
 			 SLAB_CACHE_DMA | \
+# endif
 			 SLAB_STORE_USER | \
 			 SLAB_RECLAIM_ACCOUNT | SLAB_PANIC | \
 			 SLAB_DESTROY_BY_RCU | SLAB_MEM_SPREAD | \
 			 SLAB_DEBUG_OBJECTS | SLAB_NOLEAKTRACE | SLAB_NOTRACK)
 #else
 # define CREATE_MASK	(SLAB_HWCACHE_ALIGN | \
+# ifdef CONFIG_ZONE_DMA
 			 SLAB_CACHE_DMA | \
+# endif
 			 SLAB_RECLAIM_ACCOUNT | SLAB_PANIC | \
 			 SLAB_DESTROY_BY_RCU | SLAB_MEM_SPREAD | \
 			 SLAB_DEBUG_OBJECTS | SLAB_NOLEAKTRACE | SLAB_NOTRACK)

[patch 6/6] slab: only define SLAB_CACHE_DMA for CONFIG_ZONE_DMA

[David Rientjes]

Only define SLAB_CACHE_DMA support if CONFIG_ZONE_DMA is enabled.  This
catches build errors when used on an invalid configuration.

Signed-off-by: David Rientjes <rientjes@google.com>
---
 include/linux/slab.h |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/include/linux/slab.h b/include/linux/slab.h
--- a/include/linux/slab.h
+++ b/include/linux/slab.h
@@ -20,7 +20,9 @@
 #define SLAB_RED_ZONE		0x00000400UL	/* DEBUG: Red zone objs in a cache */
 #define SLAB_POISON		0x00000800UL	/* DEBUG: Poison objects */
 #define SLAB_HWCACHE_ALIGN	0x00002000UL	/* Align objs on cache lines */
-#define SLAB_CACHE_DMA		0x00004000UL	/* Use GFP_DMA memory */
+#ifdef CONFIG_ZONE_DMA
+# define SLAB_CACHE_DMA		0x00004000UL	/* Use GFP_DMA memory */
+#endif
 #define SLAB_STORE_USER		0x00010000UL	/* DEBUG: Store the last owner for bug hunting */
 #define SLAB_PANIC		0x00040000UL	/* Panic if kmem_cache_create() fails */
 /*

Re: [patch 6/6] slab: only define SLAB_CACHE_DMA for CONFIG_ZONE_DMA

[Heiko Carstens]

On Tue, May 24, 2011 at 04:53:57PM -0700, David Rientjes wrote:
> Only define SLAB_CACHE_DMA support if CONFIG_ZONE_DMA is enabled.  This
> catches build errors when used on an invalid configuration.
> 
> Signed-off-by: David Rientjes <rientjes@google.com>
> ---
>  include/linux/slab.h |    4 +++-
>  1 files changed, 3 insertions(+), 1 deletions(-)
> 
> diff --git a/include/linux/slab.h b/include/linux/slab.h
> --- a/include/linux/slab.h
> +++ b/include/linux/slab.h
> @@ -20,7 +20,9 @@
>  #define SLAB_RED_ZONE		0x00000400UL	/* DEBUG: Red zone objs in a cache */
>  #define SLAB_POISON		0x00000800UL	/* DEBUG: Poison objects */
>  #define SLAB_HWCACHE_ALIGN	0x00002000UL	/* Align objs on cache lines */
> -#define SLAB_CACHE_DMA		0x00004000UL	/* Use GFP_DMA memory */
> +#ifdef CONFIG_ZONE_DMA
> +# define SLAB_CACHE_DMA		0x00004000UL	/* Use GFP_DMA memory */
> +#endif
>  #define SLAB_STORE_USER		0x00010000UL	/* DEBUG: Store the last owner for bug hunting */
>  #define SLAB_PANIC		0x00040000UL	/* Panic if kmem_cache_create() fails */
>  /*

Ok, now I see what you want. But please let's don't add an
#ifdef CONFIG_ZONE_DMA
to the dasd driver. Instead just re-add ZONE_DMA to 31-bit s390. Everything
would be in ZONE_DMA again and ZONE_NORMAL would be empty. Doesn't matter
if we have an additional zone, since 31-bit support isn't important anymore.

So I could add the following patch to the s390 tree, if wanted:

---
 arch/s390/Kconfig   |    2 +-
 arch/s390/mm/init.c |    2 --
 2 files changed, 1 insertion(+), 3 deletions(-)

--- a/arch/s390/Kconfig
+++ b/arch/s390/Kconfig
@@ -2,7 +2,7 @@ config MMU
 	def_bool y
 
 config ZONE_DMA
-	def_bool y if 64BIT
+	def_bool y
 
 config LOCKDEP_SUPPORT
 	def_bool y
--- a/arch/s390/mm/init.c
+++ b/arch/s390/mm/init.c
@@ -119,9 +119,7 @@ void __init paging_init(void)
 	sparse_memory_present_with_active_regions(MAX_NUMNODES);
 	sparse_init();
 	memset(max_zone_pfns, 0, sizeof(max_zone_pfns));
-#ifdef CONFIG_ZONE_DMA
 	max_zone_pfns[ZONE_DMA] = PFN_DOWN(MAX_DMA_ADDRESS);
-#endif
 	max_zone_pfns[ZONE_NORMAL] = max_low_pfn;
 	free_area_init_nodes(max_zone_pfns);
 	fault_init();

Re: [patch 6/6] slab: only define SLAB_CACHE_DMA for CONFIG_ZONE_DMA

[Christoph Lameter]

On Wed, 25 May 2011, Heiko Carstens wrote:

> Ok, now I see what you want. But please let's don't add an
> #ifdef CONFIG_ZONE_DMA
> to the dasd driver. Instead just re-add ZONE_DMA to 31-bit s390. Everything
> would be in ZONE_DMA again and ZONE_NORMAL would be empty. Doesn't matter
> if we have an additional zone, since 31-bit support isn't important anymore.

ZONE_NORMAL is normal memory. ZONE_DMA is a zone for memory restricted
to a certain range. If you remove a zone then it should be ZONE_DMA. The
core creates additional (and useless logic in you case) if you enable
ZONE_DMA.

Re: [patch 6/6] slab: only define SLAB_CACHE_DMA for CONFIG_ZONE_DMA

[David Rientjes]

On Wed, 25 May 2011, Heiko Carstens wrote:

> Ok, now I see what you want. But please let's don't add an
> #ifdef CONFIG_ZONE_DMA
> to the dasd driver. Instead just re-add ZONE_DMA to 31-bit s390. Everything
> would be in ZONE_DMA again and ZONE_NORMAL would be empty. Doesn't matter
> if we have an additional zone, since 31-bit support isn't important anymore.
> 

Ok, that's fine.  Many other architectures do not allow this to be 
disabled, so we can add s390 to the list.

> So I could add the following patch to the s390 tree, if wanted:
> 

Sure!  Please remove the CONFIG_ZONE_DMA from 
arch/s390/appldata/appldata_mem.c as well and then add my:

	Acked-by: David Rientjes <rientjes@google.com>

once you sign-it-off.

> ---
>  arch/s390/Kconfig   |    2 +-
>  arch/s390/mm/init.c |    2 --
>  2 files changed, 1 insertion(+), 3 deletions(-)
> 
> --- a/arch/s390/Kconfig
> +++ b/arch/s390/Kconfig
> @@ -2,7 +2,7 @@ config MMU
>  	def_bool y
>  
>  config ZONE_DMA
> -	def_bool y if 64BIT
> +	def_bool y
>  
>  config LOCKDEP_SUPPORT
>  	def_bool y
> --- a/arch/s390/mm/init.c
> +++ b/arch/s390/mm/init.c
> @@ -119,9 +119,7 @@ void __init paging_init(void)
>  	sparse_memory_present_with_active_regions(MAX_NUMNODES);
>  	sparse_init();
>  	memset(max_zone_pfns, 0, sizeof(max_zone_pfns));
> -#ifdef CONFIG_ZONE_DMA
>  	max_zone_pfns[ZONE_DMA] = PFN_DOWN(MAX_DMA_ADDRESS);
> -#endif
>  	max_zone_pfns[ZONE_NORMAL] = max_low_pfn;
>  	free_area_init_nodes(max_zone_pfns);
>  	fault_init();
> 

Re: [patch 1/6] s390, block: disable fixed buffer mode when DMA support is disabled

[Heiko Carstens]

On Tue, May 24, 2011 at 04:53:45PM -0700, David Rientjes wrote:
> dasd=fixedbuffers must create a SLAB_CACHE_DMA cache, which is not
> possible if CONFIG_ZONE_DMA is disabled (a supported configuration
> without 64-bit support).
> 
> If passed, emit a warning and disable fixed buffer mode.
> 
> Signed-off-by: David Rientjes <rientjes@google.com>

What's the reasoning here?
On s390 for !CONFIG_64BIT ZONE_DMA would be equal to ZONE_NORMAL,
that's why we only have ZONE_NORMAL. So I don't see that something
wouldn't work...

> ---
>  drivers/s390/block/dasd_devmap.c |    5 +++++
>  1 files changed, 5 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/s390/block/dasd_devmap.c b/drivers/s390/block/dasd_devmap.c
> --- a/drivers/s390/block/dasd_devmap.c
> +++ b/drivers/s390/block/dasd_devmap.c
> @@ -282,6 +282,11 @@ dasd_parse_keyword( char *parsestring ) {
>  		return residual_str;
>  	}
>  	if (strncmp("fixedbuffers", parsestring, length) == 0) {
> +#ifndef CONFIG_ZONE_DMA
> +		DBF_EVENT(DBF_WARNING, "%s", "DMA support disabled, "
> +					"fixed buffer mode disabled.");
> +		return residual_str;
> +#endif
>  		if (dasd_page_cache)
>  			return residual_str;
>  		dasd_page_cache =