All of lore.kernel.org
 help / color / mirror / Atom feed
From: Tejun Heo <tj@kernel.org>
To: Lai Jiangshan <laijs@cn.fujitsu.com>
Cc: Christoph Lameter <cl@linux-foundation.org>,
	Pekka Enberg <penberg@kernel.org>, Matt Mackall <mpm@selenic.com>,
	Andrew Morton <akpm@linux-foundation.org>,
	linux-kernel@vger.kernel.org, linux-mm@kvack.org
Subject: Re: [RFC PATCH 6/6] workqueue: use kmalloc_align() instead of hacking
Date: Tue, 20 Mar 2012 08:46:19 -0700	[thread overview]
Message-ID: <20120320154619.GA5684@google.com> (raw)
In-Reply-To: <1332238884-6237-7-git-send-email-laijs@cn.fujitsu.com>

On Tue, Mar 20, 2012 at 06:21:24PM +0800, Lai Jiangshan wrote:
> kmalloc_align() makes the code simpler.
> 
> Signed-off-by: Lai Jiangshan <laijs@cn.fujitsu.com>
> ---
>  kernel/workqueue.c |   23 +++++------------------
>  1 files changed, 5 insertions(+), 18 deletions(-)
> 
> diff --git a/kernel/workqueue.c b/kernel/workqueue.c
> index 5abf42f..beec5fd 100644
> --- a/kernel/workqueue.c
> +++ b/kernel/workqueue.c
> @@ -2897,20 +2897,9 @@ static int alloc_cwqs(struct workqueue_struct *wq)
>  
>  	if (!(wq->flags & WQ_UNBOUND))
>  		wq->cpu_wq.pcpu = __alloc_percpu(size, align);
> -	else {
> -		void *ptr;
> -
> -		/*
> -		 * Allocate enough room to align cwq and put an extra
> -		 * pointer at the end pointing back to the originally
> -		 * allocated pointer which will be used for free.
> -		 */
> -		ptr = kzalloc(size + align + sizeof(void *), GFP_KERNEL);
> -		if (ptr) {
> -			wq->cpu_wq.single = PTR_ALIGN(ptr, align);
> -			*(void **)(wq->cpu_wq.single + 1) = ptr;
> -		}
> -	}
> +	else
> +		wq->cpu_wq.single = kmalloc_align(size,
> +				GFP_KERNEL | __GFP_ZERO, align);
>  
>  	/* just in case, make sure it's actually aligned */
>  	BUG_ON(!IS_ALIGNED(wq->cpu_wq.v, align));
> @@ -2921,10 +2910,8 @@ static void free_cwqs(struct workqueue_struct *wq)
>  {
>  	if (!(wq->flags & WQ_UNBOUND))
>  		free_percpu(wq->cpu_wq.pcpu);
> -	else if (wq->cpu_wq.single) {
> -		/* the pointer to free is stored right after the cwq */
> -		kfree(*(void **)(wq->cpu_wq.single + 1));
> -	}
> +	else if (wq->cpu_wq.single)
> +		kfree(wq->cpu_wq.single);

Yes, this is hacky but I don't think building the whole
kmalloc_align() for only this is a good idea.  If the open coded hack
bothers you just write a simplistic wrapper somewhere.  We can make
that better integrated / more efficient when there are multiple users
of the interface, which I kinda doubt would happen.  The reason why
cwq requiring larger alignment is more historic than anything else
after all.

Thanks.

-- 
tejun

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

WARNING: multiple messages have this Message-ID (diff)
From: Tejun Heo <tj@kernel.org>
To: Lai Jiangshan <laijs@cn.fujitsu.com>
Cc: Christoph Lameter <cl@linux-foundation.org>,
	Pekka Enberg <penberg@kernel.org>, Matt Mackall <mpm@selenic.com>,
	Andrew Morton <akpm@linux-foundation.org>,
	linux-kernel@vger.kernel.org, linux-mm@kvack.org
Subject: Re: [RFC PATCH 6/6] workqueue: use kmalloc_align() instead of hacking
Date: Tue, 20 Mar 2012 08:46:19 -0700	[thread overview]
Message-ID: <20120320154619.GA5684@google.com> (raw)
In-Reply-To: <1332238884-6237-7-git-send-email-laijs@cn.fujitsu.com>

On Tue, Mar 20, 2012 at 06:21:24PM +0800, Lai Jiangshan wrote:
> kmalloc_align() makes the code simpler.
> 
> Signed-off-by: Lai Jiangshan <laijs@cn.fujitsu.com>
> ---
>  kernel/workqueue.c |   23 +++++------------------
>  1 files changed, 5 insertions(+), 18 deletions(-)
> 
> diff --git a/kernel/workqueue.c b/kernel/workqueue.c
> index 5abf42f..beec5fd 100644
> --- a/kernel/workqueue.c
> +++ b/kernel/workqueue.c
> @@ -2897,20 +2897,9 @@ static int alloc_cwqs(struct workqueue_struct *wq)
>  
>  	if (!(wq->flags & WQ_UNBOUND))
>  		wq->cpu_wq.pcpu = __alloc_percpu(size, align);
> -	else {
> -		void *ptr;
> -
> -		/*
> -		 * Allocate enough room to align cwq and put an extra
> -		 * pointer at the end pointing back to the originally
> -		 * allocated pointer which will be used for free.
> -		 */
> -		ptr = kzalloc(size + align + sizeof(void *), GFP_KERNEL);
> -		if (ptr) {
> -			wq->cpu_wq.single = PTR_ALIGN(ptr, align);
> -			*(void **)(wq->cpu_wq.single + 1) = ptr;
> -		}
> -	}
> +	else
> +		wq->cpu_wq.single = kmalloc_align(size,
> +				GFP_KERNEL | __GFP_ZERO, align);
>  
>  	/* just in case, make sure it's actually aligned */
>  	BUG_ON(!IS_ALIGNED(wq->cpu_wq.v, align));
> @@ -2921,10 +2910,8 @@ static void free_cwqs(struct workqueue_struct *wq)
>  {
>  	if (!(wq->flags & WQ_UNBOUND))
>  		free_percpu(wq->cpu_wq.pcpu);
> -	else if (wq->cpu_wq.single) {
> -		/* the pointer to free is stored right after the cwq */
> -		kfree(*(void **)(wq->cpu_wq.single + 1));
> -	}
> +	else if (wq->cpu_wq.single)
> +		kfree(wq->cpu_wq.single);

Yes, this is hacky but I don't think building the whole
kmalloc_align() for only this is a good idea.  If the open coded hack
bothers you just write a simplistic wrapper somewhere.  We can make
that better integrated / more efficient when there are multiple users
of the interface, which I kinda doubt would happen.  The reason why
cwq requiring larger alignment is more historic than anything else
after all.

Thanks.

-- 
tejun

  parent reply	other threads:[~2012-03-20 15:46 UTC|newest]

Thread overview: 55+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2012-03-20 10:21 [RFC PATCH 0/6] add kmalloc_align() Lai Jiangshan
2012-03-20 10:21 ` Lai Jiangshan
2012-03-20 10:21 ` [RFC PATCH 1/6] kenrel.h: add ALIGN_OF_LAST_BIT() Lai Jiangshan
2012-03-20 10:21   ` Lai Jiangshan
2012-03-20 11:32   ` Michal Nazarewicz
2012-03-20 11:32     ` Michal Nazarewicz
2012-03-20 14:03     ` Alexey Dobriyan
2012-03-20 14:03       ` Alexey Dobriyan
2012-03-20 14:08       ` Christoph Lameter
2012-03-20 14:20     ` Peter Seebach
2012-03-20 14:20       ` Peter Seebach
2012-03-20 10:21 ` [RFC PATCH 2/6] slub: add kmalloc_align() Lai Jiangshan
2012-03-20 10:21   ` Lai Jiangshan
2012-03-20 14:14   ` Christoph Lameter
2012-03-20 14:14     ` Christoph Lameter
2012-03-20 14:21     ` Christoph Lameter
2012-03-20 14:21       ` Christoph Lameter
2012-03-20 10:21 ` [RFC PATCH 3/6] slab: " Lai Jiangshan
2012-03-20 10:21   ` Lai Jiangshan
2012-03-20 10:21 ` [RFC PATCH 4/6] slob: don't couple the header size with the alignment Lai Jiangshan
2012-03-20 10:21   ` Lai Jiangshan
2012-03-20 10:21 ` [RFC PATCH 5/6] slob: add kmalloc_align() Lai Jiangshan
2012-03-20 10:21   ` Lai Jiangshan
2012-03-20 10:21 ` [RFC PATCH 6/6] workqueue: use kmalloc_align() instead of hacking Lai Jiangshan
2012-03-20 10:21   ` Lai Jiangshan
2012-03-20 15:15   ` Christoph Lameter
2012-03-20 15:15     ` Christoph Lameter
2012-03-20 15:46   ` Tejun Heo [this message]
2012-03-20 15:46     ` Tejun Heo
2012-03-21  3:02     ` Lai Jiangshan
2012-03-21  3:02       ` Lai Jiangshan
2012-03-21  5:14       ` Tejun Heo
2012-03-21  5:14         ` Tejun Heo
2012-03-21 14:12         ` Patch workqueue: create new slab cache " Christoph Lameter
2012-03-21 14:12           ` Christoph Lameter
2012-03-21 14:49           ` Eric Dumazet
2012-03-21 14:49             ` Eric Dumazet
2012-03-21 15:03             ` Christoph Lameter
2012-03-21 15:03               ` Christoph Lameter
2012-03-21 16:04               ` Eric Dumazet
2012-03-21 16:04                 ` Eric Dumazet
2012-03-21 17:54                 ` Christoph Lameter
2012-03-21 17:54                   ` Christoph Lameter
2012-03-21 18:05                   ` Eric Dumazet
2012-03-21 18:05                     ` Eric Dumazet
2012-03-21 18:20                     ` Christoph Lameter
2012-03-21 18:20                       ` Christoph Lameter
2012-03-21 16:09           ` Tejun Heo
2012-03-21 16:09             ` Tejun Heo
2012-03-21 17:56             ` Christoph Lameter
2012-03-21 17:56               ` Christoph Lameter
2012-03-21 13:45       ` [RFC PATCH 6/6] workqueue: use kmalloc_align() " Christoph Lameter
2012-03-21 13:45         ` Christoph Lameter
2012-03-26  2:00         ` Lai Jiangshan
2012-03-26  2:00           ` Lai Jiangshan

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20120320154619.GA5684@google.com \
    --to=tj@kernel.org \
    --cc=akpm@linux-foundation.org \
    --cc=cl@linux-foundation.org \
    --cc=laijs@cn.fujitsu.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mm@kvack.org \
    --cc=mpm@selenic.com \
    --cc=penberg@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.