[PATCH] msi: Only keep one msi_desc in each slab entry.

[PATCH] msi: Only keep one msi_desc in each slab entry.

[Eric W. Biederman]

It looks like someone confused kmem_cache_create with a different
allocator and was attempting to give it knowledge of how many cache
entries there were.

With the unfortunate result that each slab entry was big enough to
hold every irq.

Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>
---
 drivers/pci/msi.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
index 0cd4a3e..082e942 100644
--- a/drivers/pci/msi.c
+++ b/drivers/pci/msi.c
@@ -40,13 +40,13 @@ msi_register(struct msi_ops *ops)
 
 static void msi_cache_ctor(void *p, kmem_cache_t *cache, unsigned long flags)
 {
-	memset(p, 0, NR_IRQS * sizeof(struct msi_desc));
+	memset(p, 0, sizeof(struct msi_desc));
 }
 
 static int msi_cache_init(void)
 {
 	msi_cachep = kmem_cache_create("msi_cache",
-			NR_IRQS * sizeof(struct msi_desc),
+			sizeof(struct msi_desc),
 		       	0, SLAB_HWCACHE_ALIGN, msi_cache_ctor, NULL);
 	if (!msi_cachep)
 		return -ENOMEM;
-- 
1.4.1.gac83a

Re: [PATCH] msi: Only keep one msi_desc in each slab entry.

[Pekka Enberg]

On 7/11/06, Eric W. Biederman <ebiederm@xmission.com> wrote:
>
> It looks like someone confused kmem_cache_create with a different
> allocator and was attempting to give it knowledge of how many cache
> entries there were.
>
> With the unfortunate result that each slab entry was big enough to
> hold every irq.
>
> Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>
> ---
>  drivers/pci/msi.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
> index 0cd4a3e..082e942 100644
> --- a/drivers/pci/msi.c
> +++ b/drivers/pci/msi.c
> @@ -40,13 +40,13 @@ msi_register(struct msi_ops *ops)
>
>  static void msi_cache_ctor(void *p, kmem_cache_t *cache, unsigned long flags)
>  {
> -       memset(p, 0, NR_IRQS * sizeof(struct msi_desc));
> +       memset(p, 0, sizeof(struct msi_desc));

You can use kmem_cache_zalloc() for this.

>  }
>
>  static int msi_cache_init(void)
>  {
>         msi_cachep = kmem_cache_create("msi_cache",
> -                       NR_IRQS * sizeof(struct msi_desc),
> +                       sizeof(struct msi_desc),
>                         0, SLAB_HWCACHE_ALIGN, msi_cache_ctor, NULL);
>         if (!msi_cachep)
>                 return -ENOMEM;
> --
> 1.4.1.gac83a
>
> -
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
>

Re: [PATCH] msi: Only keep one msi_desc in each slab entry.

[Eric W. Biederman]

"Pekka Enberg" <penberg@cs.helsinki.fi> writes:

> On 7/11/06, Eric W. Biederman <ebiederm@xmission.com> wrote:
>>
>> It looks like someone confused kmem_cache_create with a different
>> allocator and was attempting to give it knowledge of how many cache
>> entries there were.
>>
>> With the unfortunate result that each slab entry was big enough to
>> hold every irq.
>>
>> Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>
>> ---
>>  drivers/pci/msi.c |    4 ++--
>>  1 files changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
>> index 0cd4a3e..082e942 100644
>> --- a/drivers/pci/msi.c
>> +++ b/drivers/pci/msi.c
>> @@ -40,13 +40,13 @@ msi_register(struct msi_ops *ops)
>>
>> static void msi_cache_ctor(void *p, kmem_cache_t *cache, unsigned long flags)
>>  {
>> -       memset(p, 0, NR_IRQS * sizeof(struct msi_desc));
>> +       memset(p, 0, sizeof(struct msi_desc));
>
> You can use kmem_cache_zalloc() for this.

Please look at what the code changes.
Please recognize how very bad the current code is behaving.

As for the rest sure go ahead and create a patch to address it
but that really is a separate issue and thus a separate patch.

I'm just trying to keep the kernel from calling BUG_ON the first
time a msi irq is allocated on a kernel with a maximum NR_CPUS
configuration, and from wasting memory the rest of the time.

Or you know how bad the msi code is when every patch to fix a major
issue is followed up comments on how to improve the code even further.

Eric

>>  }
>>
>>  static int msi_cache_init(void)
>>  {
>>         msi_cachep = kmem_cache_create("msi_cache",
>> -                       NR_IRQS * sizeof(struct msi_desc),
>> +                       sizeof(struct msi_desc),
>>                         0, SLAB_HWCACHE_ALIGN, msi_cache_ctor, NULL);
>>         if (!msi_cachep)
>>                 return -ENOMEM;
>> --
>> 1.4.1.gac83a
>>
>> -
>> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>> Please read the FAQ at  http://www.tux.org/lkml/
>>

Re: [PATCH] msi: Only keep one msi_desc in each slab entry.

[Pekka J Enberg]

On Tue, 11 Jul 2006, Eric W. Biederman wrote:
> Please look at what the code changes.
> Please recognize how very bad the current code is behaving.

Yes, there's plenty of slab confusion going on.

On Tue, 11 Jul 2006, Eric W. Biederman wrote:
> As for the rest sure go ahead and create a patch to address it
> but that really is a separate issue and thus a separate patch.
> 
> I'm just trying to keep the kernel from calling BUG_ON the first
> time a msi irq is allocated on a kernel with a maximum NR_CPUS
> configuration, and from wasting memory the rest of the time.
> 
> Or you know how bad the msi code is when every patch to fix a major
> issue is followed up comments on how to improve the code even further.

Ok.

Signed-off-by: Pekka Enberg <penberg@cs.helsinki.fi>
---

diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
index 36bc7c4..77b08ee 100644
--- a/drivers/pci/msi.c
+++ b/drivers/pci/msi.c
@@ -45,16 +45,11 @@ msi_register(struct msi_ops *ops)
 	return 0;
 }
 
-static void msi_cache_ctor(void *p, kmem_cache_t *cache, unsigned long flags)
-{
-	memset(p, 0, NR_IRQS * sizeof(struct msi_desc));
-}
-
 static int msi_cache_init(void)
 {
 	msi_cachep = kmem_cache_create("msi_cache",
-			NR_IRQS * sizeof(struct msi_desc),
-		       	0, SLAB_HWCACHE_ALIGN, msi_cache_ctor, NULL);
+			sizeof(struct msi_desc),
+		       	0, SLAB_HWCACHE_ALIGN, NULL, NULL);
 	if (!msi_cachep)
 		return -ENOMEM;
 
@@ -402,11 +397,10 @@ static struct msi_desc* alloc_msi_entry(
 {
 	struct msi_desc *entry;
 
-	entry = kmem_cache_alloc(msi_cachep, SLAB_KERNEL);
+	entry = kmem_cache_zalloc(msi_cachep, GFP_KERNEL);
 	if (!entry)
 		return NULL;
 
-	memset(entry, 0, sizeof(struct msi_desc));
 	entry->link.tail = entry->link.head = 0;	/* single message */
 	entry->dev = NULL;
 

Re: [PATCH] msi: Only keep one msi_desc in each slab entry.

[Eric W. Biederman]

Pekka J Enberg <penberg@cs.Helsinki.FI> writes:

> On Tue, 11 Jul 2006, Eric W. Biederman wrote:
>> Please look at what the code changes.
>> Please recognize how very bad the current code is behaving.
>
> Yes, there's plenty of slab confusion going on.
>
> On Tue, 11 Jul 2006, Eric W. Biederman wrote:
>> As for the rest sure go ahead and create a patch to address it
>> but that really is a separate issue and thus a separate patch.
>> 
>> I'm just trying to keep the kernel from calling BUG_ON the first
>> time a msi irq is allocated on a kernel with a maximum NR_CPUS
>> configuration, and from wasting memory the rest of the time.
>> 
>> Or you know how bad the msi code is when every patch to fix a major
>> issue is followed up comments on how to improve the code even further.
>
> Ok.

Looks good to me. 

> Signed-off-by: Pekka Enberg <penberg@cs.helsinki.fi>
> ---
>
> diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
> index 36bc7c4..77b08ee 100644
> --- a/drivers/pci/msi.c
> +++ b/drivers/pci/msi.c
> @@ -45,16 +45,11 @@ msi_register(struct msi_ops *ops)
>  	return 0;
>  }
>  
> -static void msi_cache_ctor(void *p, kmem_cache_t *cache, unsigned long flags)
> -{
> -	memset(p, 0, NR_IRQS * sizeof(struct msi_desc));
> -}
> -
>  static int msi_cache_init(void)
>  {
>  	msi_cachep = kmem_cache_create("msi_cache",
> -			NR_IRQS * sizeof(struct msi_desc),
> -		       	0, SLAB_HWCACHE_ALIGN, msi_cache_ctor, NULL);
> +			sizeof(struct msi_desc),
> +		       	0, SLAB_HWCACHE_ALIGN, NULL, NULL);
>  	if (!msi_cachep)
>  		return -ENOMEM;
>  
> @@ -402,11 +397,10 @@ static struct msi_desc* alloc_msi_entry(
>  {
>  	struct msi_desc *entry;
>  
> -	entry = kmem_cache_alloc(msi_cachep, SLAB_KERNEL);
> +	entry = kmem_cache_zalloc(msi_cachep, GFP_KERNEL);
>  	if (!entry)
>  		return NULL;
>  
> -	memset(entry, 0, sizeof(struct msi_desc));
>  	entry->link.tail = entry->link.head = 0;	/* single message */
>  	entry->dev = NULL;
>