diff mbox

[RFC,v2] genalloc:add an gen_pool_first_fit_align algo to genalloc

Message ID 1437991074-35377-1-git-send-email-qiang.zhao@freescale.com
State RFC
Delegated to: Scott Wood
Headers show

Commit Message

Zhao Qiang July 27, 2015, 9:57 a.m. UTC
Bytes alignment is required to manage some special ram,
so add gen_pool_first_fit_align to genalloc.
User should define data structure
struct data {
	int align;
	struct gen_pool *pool;
}
align is the number of  bytes alignment,
pool points to gen_pool which include data.

Signed-off-by: Zhao Qiang <qiang.zhao@freescale.com>
---
*v2:
changes:
title has been modified, original patch link: 
http://patchwork.ozlabs.org/patch/493297/

original patch add a func gen_pool_alloc_align, 
then pass alignment to it as an parameter.
after discussing with lauraa and scott, they recommend 
to pass alignment as part of data based on 
commit message for ca279cf1065fb689abea1dc7d8c11787729bb185 which adds "data":

"As I can't predict all the possible requirements/needs for all allocation    
uses cases, I add a "free" field 'void *data' to pass any needed     
information to the allocation function.  For example 'data' could be used     
to handle a structure where you store the alignment, the expected memory     
bank, the requester device, or any information that could influence the     
allocation algorithm."




 include/linux/genalloc.h |  4 ++++
 lib/genalloc.c           | 25 +++++++++++++++++++++++++
 2 files changed, 29 insertions(+)

Comments

Vladimir Zapolskiy July 27, 2015, 4:49 p.m. UTC | #1
Hello Zhao,

On 27.07.2015 12:57, Zhao Qiang wrote:
> Bytes alignment is required to manage some special ram,
> so add gen_pool_first_fit_align to genalloc.
> User should define data structure
> struct data {
> 	int align;
> 	struct gen_pool *pool;
> }
> align is the number of  bytes alignment,
> pool points to gen_pool which include data.
> 
> Signed-off-by: Zhao Qiang <qiang.zhao@freescale.com>
> ---
> *v2:
> changes:
> title has been modified, original patch link: 
> http://patchwork.ozlabs.org/patch/493297/
> 
> original patch add a func gen_pool_alloc_align, 
> then pass alignment to it as an parameter.
> after discussing with lauraa and scott, they recommend 
> to pass alignment as part of data based on 
> commit message for ca279cf1065fb689abea1dc7d8c11787729bb185 which adds "data":
> 
> "As I can't predict all the possible requirements/needs for all allocation    
> uses cases, I add a "free" field 'void *data' to pass any needed     
> information to the allocation function.  For example 'data' could be used     
> to handle a structure where you store the alignment, the expected memory     
> bank, the requester device, or any information that could influence the     
> allocation algorithm."
> 
> 
> 
> 
>  include/linux/genalloc.h |  4 ++++
>  lib/genalloc.c           | 25 +++++++++++++++++++++++++
>  2 files changed, 29 insertions(+)
> 
> diff --git a/include/linux/genalloc.h b/include/linux/genalloc.h
> index 1ccaab4..b85d0f8 100644
> --- a/include/linux/genalloc.h
> +++ b/include/linux/genalloc.h
> @@ -110,6 +110,10 @@ extern void gen_pool_set_algo(struct gen_pool *pool, genpool_algo_t algo,
>  extern unsigned long gen_pool_first_fit(unsigned long *map, unsigned long size,
>  		unsigned long start, unsigned int nr, void *data);
>  
> +extern unsigned long gen_pool_first_fit_align(unsigned long *map,
> +		unsigned long size, unsigned long start, unsigned int nr,
> +		void *data);
> +
>  extern unsigned long gen_pool_first_fit_order_align(unsigned long *map,
>  		unsigned long size, unsigned long start, unsigned int nr,
>  		void *data);
> diff --git a/lib/genalloc.c b/lib/genalloc.c
> index d214866..e6608cd 100644
> --- a/lib/genalloc.c
> +++ b/lib/genalloc.c
> @@ -509,6 +509,31 @@ unsigned long gen_pool_first_fit(unsigned long *map, unsigned long size,
>  EXPORT_SYMBOL(gen_pool_first_fit);
>  
>  /**
> + * gen_pool_first_fit_align - find the first available region
> + * of memory matching the size requirement (no alignment constraint)

no alignment constraint?

> + * @map: The address to base the search on
> + * @size: The bitmap size in bits
> + * @start: The bitnumber to start searching at
> + * @nr: The number of zeroed bits we're looking for
> + * @data: additional data - unused

unused?

> + */
> +unsigned long gen_pool_first_fit_align(unsigned long *map, unsigned long size,
> +		unsigned long start, unsigned int nr, void *data)
> +{
> +	unsigned long align_mask;
> +	int order;
> +
> +	if (data && data->pool) {

"data" type is (void *), missing cast?

> +		order = data->pool->min_alloc_order;
> +		align_mask = ((data->align + (1UL << order) - 1) >> order) - 1;
> +	} else {
> +		pr_err("no data or data->pool\n");
> +	}
> +	return bitmap_find_next_zero_area(map, size, start, nr, align_mask);

You may end up in a situation, when align_mask is undefined, I believe
bitmap_find_next_zero_area() should not be called on error path.

> +}
> +EXPORT_SYMBOL(gen_pool_first_fit_algin);
> +
> +/**
>   * gen_pool_first_fit_order_align - find the first available region
>   * of memory matching the size requirement. The region will be aligned
>   * to the order of the size specified.
> 

--
With best wishes,
Vladimir
Scott Wood July 27, 2015, 9:20 p.m. UTC | #2
On Mon, 2015-07-27 at 17:57 +0800, Zhao Qiang wrote:
> diff --git a/lib/genalloc.c b/lib/genalloc.c
> index d214866..e6608cd 100644
> --- a/lib/genalloc.c
> +++ b/lib/genalloc.c
> @@ -509,6 +509,31 @@ unsigned long gen_pool_first_fit(unsigned long *map, 
> unsigned long size,
>  EXPORT_SYMBOL(gen_pool_first_fit);
>  
>  /**
> + * gen_pool_first_fit_align - find the first available region
> + * of memory matching the size requirement (no alignment constraint)
> + * @map: The address to base the search on
> + * @size: The bitmap size in bits
> + * @start: The bitnumber to start searching at
> + * @nr: The number of zeroed bits we're looking for
> + * @data: additional data - unused
> + */
> +unsigned long gen_pool_first_fit_align(unsigned long *map, unsigned long 
> size,
> +             unsigned long start, unsigned int nr, void *data)
> +{
> +     unsigned long align_mask;
> +     int order;
> +
> +     if (data && data->pool) {

There is no way that this compiles.  You can't dereference a void pointer.

Please test your code before submitting, even for an RFC.

> +             order = data->pool->min_alloc_order;

I don't think pool belongs in data.  It's fundamental enough that, if a 
pointer to pool is needed, it should be an argument to the algorithm.

> +             align_mask = ((data->align + (1UL << order) - 1) >> order) - 1;
> +     } else {
> +             pr_err("no data or data->pool\n");
> +     }

This is way too vague and unobtrusive of an error message, and also not rate-
limited, etc.  I wouldn't bother checking at all.  Just let it crash on the 
developer's machine if they use this without passing in data.

Where's the part that adds the ability to pass in data to each allocation 
call, as per the previous discussion?

-Scott
Zhao Qiang July 28, 2015, 5:32 a.m. UTC | #3
On Tue, 2015-07-28 at 5:21, Scott Wood wrote:
> -----Original Message-----

> From: Wood Scott-B07421

> Sent: Tuesday, July 28, 2015 5:21 AM

> To: Zhao Qiang-B45475

> Cc: lauraa@codeaurora.org; linux-kernel@vger.kernel.org; linuxppc-

> dev@lists.ozlabs.org; akpm@linux-foundation.org; olof@lixom.net;

> catalin.marinas@arm.com; Xie Xiaobo-R63061

> Subject: Re: [RFC v2] genalloc:add an gen_pool_first_fit_align algo to

> genalloc

> 

> On Mon, 2015-07-27 at 17:57 +0800, Zhao Qiang wrote:

> 

> Where's the part that adds the ability to pass in data to each allocation

> call, as per the previous discussion?


You means to use gen_pool_alloc_data()?
Previously you said that the format of data is algorithm-specific,
So I think it is better to handle data in algorithm function.

If you still prefer gen_pool_alloc_data(), I will modify it.
But there still are details I want to confirm with you.
1. If use gen_pool_alloc_data(), should I pass data as a parameter?
2. Should I count align_mask in gen_pool_alloc_data(), meanwhile, add 
   a align_mask to data as a member?
3. where to define the data, in genalloc.h or caller layer?

> 

> -Scott
Scott Wood July 29, 2015, 4:19 p.m. UTC | #4
On Tue, 2015-07-28 at 00:32 -0500, Zhao Qiang-B45475 wrote:
> On Tue, 2015-07-28 at 5:21, Scott Wood wrote:
> > -----Original Message-----
> > From: Wood Scott-B07421
> > Sent: Tuesday, July 28, 2015 5:21 AM
> > To: Zhao Qiang-B45475
> > Cc: lauraa@codeaurora.org; linux-kernel@vger.kernel.org; linuxppc-
> > dev@lists.ozlabs.org; akpm@linux-foundation.org; olof@lixom.net;
> > catalin.marinas@arm.com; Xie Xiaobo-R63061
> > Subject: Re: [RFC v2] genalloc:add an gen_pool_first_fit_align algo to
> > genalloc
> > 
> > On Mon, 2015-07-27 at 17:57 +0800, Zhao Qiang wrote:
> > 
> > Where's the part that adds the ability to pass in data to each allocation
> > call, as per the previous discussion?
> 
> You means to use gen_pool_alloc_data()?

Yes.

> Previously you said that the format of data is algorithm-specific,
> So I think it is better to handle data in algorithm function.

It is a channel for communication from the API caller to the algorithm.

> If you still prefer gen_pool_alloc_data(), I will modify it.
> But there still are details I want to confirm with you.
> 1. If use gen_pool_alloc_data(), should I pass data as a parameter?

Yes.

> 2. Should I count align_mask in gen_pool_alloc_data(), meanwhile, add 
>    a align_mask to data as a member?

gen_pool_alloc_data() should just pass data to the algorithm.  The algorithm 
should calculate align_mask based on align.  I don't think exposing 
align_mask to API users would be very friendly.

> 3. where to define the data, in genalloc.h or caller layer?

Same place as where the algorithm function is declared.

-Scott
Zhao Qiang July 30, 2015, 1:27 a.m. UTC | #5
On Thu, 2015-07-30 at 5:21, Scott Wood wrote:
> -----Original Message-----

> From: Wood Scott-B07421

> Sent: Thursday, July 30, 2015 12:19 AM

> To: Zhao Qiang-B45475

> Cc: lauraa@codeaurora.org; linux-kernel@vger.kernel.org; linuxppc-

> dev@lists.ozlabs.org; akpm@linux-foundation.org; olof@lixom.net;

> catalin.marinas@arm.com; Xie Xiaobo-R63061

> Subject: Re: [RFC v2] genalloc:add an gen_pool_first_fit_align algo to

> genalloc

> 

> On Tue, 2015-07-28 at 00:32 -0500, Zhao Qiang-B45475 wrote:

> > On Tue, 2015-07-28 at 5:21, Scott Wood wrote:

> > > -----Original Message-----

> > > From: Wood Scott-B07421

> > > Sent: Tuesday, July 28, 2015 5:21 AM

> > > To: Zhao Qiang-B45475

> > > Cc: lauraa@codeaurora.org; linux-kernel@vger.kernel.org; linuxppc-

> > > dev@lists.ozlabs.org; akpm@linux-foundation.org; olof@lixom.net;

> > > catalin.marinas@arm.com; Xie Xiaobo-R63061

> > > Subject: Re: [RFC v2] genalloc:add an gen_pool_first_fit_align algo

> > > to genalloc

> > >

> > > On Mon, 2015-07-27 at 17:57 +0800, Zhao Qiang wrote:

> > >

> > > Where's the part that adds the ability to pass in data to each

> > > allocation call, as per the previous discussion?

> >

> > You means to use gen_pool_alloc_data()?

> 

> Yes.

> 

> > Previously you said that the format of data is algorithm-specific, So

> > I think it is better to handle data in algorithm function.

> 

> It is a channel for communication from the API caller to the algorithm.

> 

> > If you still prefer gen_pool_alloc_data(), I will modify it.

> > But there still are details I want to confirm with you.

> > 1. If use gen_pool_alloc_data(), should I pass data as a parameter?

> 

> Yes.

> 

> > 2. Should I count align_mask in gen_pool_alloc_data(), meanwhile, add

> >    a align_mask to data as a member?

> 

> gen_pool_alloc_data() should just pass data to the algorithm.  The

> algorithm should calculate align_mask based on align.  I don't think

> exposing align_mask to API users would be very friendly.


If calculate align_mask in algorithm, I need to get pool->min_alloc_order in algorithm,
Like:
               order = data->pool->min_alloc_order;
               align_mask = ((data->align + (1UL << order) - 1) >> order) - 1; 
so I add pool to structure data as a member. Is there any other better idea? 

> 

> > 3. where to define the data, in genalloc.h or caller layer?

> 

> Same place as where the algorithm function is declared.

> 

> -Scott

> 

-Zhao Qiang
Scott Wood July 30, 2015, 5:06 p.m. UTC | #6
On Wed, 2015-07-29 at 20:27 -0500, Zhao Qiang-B45475 wrote:
> On Thu, 2015-07-30 at 5:21, Scott Wood wrote:
> > -----Original Message-----
> > From: Wood Scott-B07421
> > Sent: Thursday, July 30, 2015 12:19 AM
> > To: Zhao Qiang-B45475
> > Cc: lauraa@codeaurora.org; linux-kernel@vger.kernel.org; linuxppc-
> > dev@lists.ozlabs.org; akpm@linux-foundation.org; olof@lixom.net;
> > catalin.marinas@arm.com; Xie Xiaobo-R63061
> > Subject: Re: [RFC v2] genalloc:add an gen_pool_first_fit_align algo to
> > genalloc
> > 
> > On Tue, 2015-07-28 at 00:32 -0500, Zhao Qiang-B45475 wrote:
> > > On Tue, 2015-07-28 at 5:21, Scott Wood wrote:
> > > > -----Original Message-----
> > > > From: Wood Scott-B07421
> > > > Sent: Tuesday, July 28, 2015 5:21 AM
> > > > To: Zhao Qiang-B45475
> > > > Cc: lauraa@codeaurora.org; linux-kernel@vger.kernel.org; linuxppc-
> > > > dev@lists.ozlabs.org; akpm@linux-foundation.org; olof@lixom.net;
> > > > catalin.marinas@arm.com; Xie Xiaobo-R63061
> > > > Subject: Re: [RFC v2] genalloc:add an gen_pool_first_fit_align algo
> > > > to genalloc
> > > > 
> > > > On Mon, 2015-07-27 at 17:57 +0800, Zhao Qiang wrote:
> > > > 
> > > > Where's the part that adds the ability to pass in data to each
> > > > allocation call, as per the previous discussion?
> > > 
> > > You means to use gen_pool_alloc_data()?
> > 
> > Yes.
> > 
> > > Previously you said that the format of data is algorithm-specific, So
> > > I think it is better to handle data in algorithm function.
> > 
> > It is a channel for communication from the API caller to the algorithm.
> > 
> > > If you still prefer gen_pool_alloc_data(), I will modify it.
> > > But there still are details I want to confirm with you.
> > > 1. If use gen_pool_alloc_data(), should I pass data as a parameter?
> > 
> > Yes.
> > 
> > > 2. Should I count align_mask in gen_pool_alloc_data(), meanwhile, add
> > >    a align_mask to data as a member?
> > 
> > gen_pool_alloc_data() should just pass data to the algorithm.  The
> > algorithm should calculate align_mask based on align.  I don't think
> > exposing align_mask to API users would be very friendly.
> 
> If calculate align_mask in algorithm, I need to get pool->min_alloc_order 
> in algorithm,
> Like:
>                order = data->pool->min_alloc_order;
>                align_mask = ((data->align + (1UL << order) - 1) >> order) - 
> 1; 
> so I add pool to structure data as a member. Is there any other better 
> idea? 

Pass pool as a parameter to the algorithm.

-Scott
diff mbox

Patch

diff --git a/include/linux/genalloc.h b/include/linux/genalloc.h
index 1ccaab4..b85d0f8 100644
--- a/include/linux/genalloc.h
+++ b/include/linux/genalloc.h
@@ -110,6 +110,10 @@  extern void gen_pool_set_algo(struct gen_pool *pool, genpool_algo_t algo,
 extern unsigned long gen_pool_first_fit(unsigned long *map, unsigned long size,
 		unsigned long start, unsigned int nr, void *data);
 
+extern unsigned long gen_pool_first_fit_align(unsigned long *map,
+		unsigned long size, unsigned long start, unsigned int nr,
+		void *data);
+
 extern unsigned long gen_pool_first_fit_order_align(unsigned long *map,
 		unsigned long size, unsigned long start, unsigned int nr,
 		void *data);
diff --git a/lib/genalloc.c b/lib/genalloc.c
index d214866..e6608cd 100644
--- a/lib/genalloc.c
+++ b/lib/genalloc.c
@@ -509,6 +509,31 @@  unsigned long gen_pool_first_fit(unsigned long *map, unsigned long size,
 EXPORT_SYMBOL(gen_pool_first_fit);
 
 /**
+ * gen_pool_first_fit_align - find the first available region
+ * of memory matching the size requirement (no alignment constraint)
+ * @map: The address to base the search on
+ * @size: The bitmap size in bits
+ * @start: The bitnumber to start searching at
+ * @nr: The number of zeroed bits we're looking for
+ * @data: additional data - unused
+ */
+unsigned long gen_pool_first_fit_align(unsigned long *map, unsigned long size,
+		unsigned long start, unsigned int nr, void *data)
+{
+	unsigned long align_mask;
+	int order;
+
+	if (data && data->pool) {
+		order = data->pool->min_alloc_order;
+		align_mask = ((data->align + (1UL << order) - 1) >> order) - 1;
+	} else {
+		pr_err("no data or data->pool\n");
+	}
+	return bitmap_find_next_zero_area(map, size, start, nr, align_mask);
+}
+EXPORT_SYMBOL(gen_pool_first_fit_algin);
+
+/**
  * gen_pool_first_fit_order_align - find the first available region
  * of memory matching the size requirement. The region will be aligned
  * to the order of the size specified.