All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] mm: change size_t to unsigned int for cma_alloc
@ 2021-03-29 18:25 Minchan Kim
  2021-03-29 18:44 ` Matthew Wilcox
  0 siblings, 1 reply; 6+ messages in thread
From: Minchan Kim @ 2021-03-29 18:25 UTC (permalink / raw)
  To: Andrew Morton; +Cc: linux-mm, LKML, Minchan Kim, Matthew Wilcox

size_t in cma_alloc is confusing since it makes people think
it's byte count, not pages. Fix it.

Link: https://lore.kernel.org/linux-mm/20210324043434.GP1719932@casper.infradead.org/
Cc: Matthew Wilcox <willy@infradead.org>
Signed-off-by: Minchan Kim <minchan@kernel.org>
---
 include/linux/cma.h | 2 +-
 mm/cma.c            | 8 ++++----
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/include/linux/cma.h b/include/linux/cma.h
index 217999c8a762..a873edc20ca2 100644
--- a/include/linux/cma.h
+++ b/include/linux/cma.h
@@ -44,7 +44,7 @@ extern int cma_init_reserved_mem(phys_addr_t base, phys_addr_t size,
 					unsigned int order_per_bit,
 					const char *name,
 					struct cma **res_cma);
-extern struct page *cma_alloc(struct cma *cma, size_t count, unsigned int align,
+extern struct page *cma_alloc(struct cma *cma, unsigned int count, unsigned int align,
 			      bool no_warn);
 extern bool cma_release(struct cma *cma, const struct page *pages, unsigned int count);
 
diff --git a/mm/cma.c b/mm/cma.c
index 08c45157911a..24dc01e26d45 100644
--- a/mm/cma.c
+++ b/mm/cma.c
@@ -423,21 +423,21 @@ static inline void cma_debug_show_areas(struct cma *cma) { }
  * This function allocates part of contiguous memory on specific
  * contiguous memory area.
  */
-struct page *cma_alloc(struct cma *cma, size_t count, unsigned int align,
+struct page *cma_alloc(struct cma *cma, unsigned int count, unsigned int align,
 		       bool no_warn)
 {
 	unsigned long mask, offset;
 	unsigned long pfn = -1;
 	unsigned long start = 0;
 	unsigned long bitmap_maxno, bitmap_no, bitmap_count;
-	size_t i;
+	unsigned int i;
 	struct page *page = NULL;
 	int ret = -ENOMEM;
 
 	if (!cma || !cma->count || !cma->bitmap)
 		goto out;
 
-	pr_debug("%s(cma %p, count %zu, align %d)\n", __func__, (void *)cma,
+	pr_debug("%s(cma %p, count %u, align %d)\n", __func__, (void *)cma,
 		 count, align);
 
 	if (!count)
@@ -500,7 +500,7 @@ struct page *cma_alloc(struct cma *cma, size_t count, unsigned int align,
 	}
 
 	if (ret && !no_warn) {
-		pr_err_ratelimited("%s: %s: alloc failed, req-size: %zu pages, ret: %d\n",
+		pr_err_ratelimited("%s: %s: alloc failed, req-size: %u pages, ret: %d\n",
 				   __func__, cma->name, count, ret);
 		cma_debug_show_areas(cma);
 	}
-- 
2.31.0.291.g576ba9dcdaf-goog


^ permalink raw reply related	[flat|nested] 6+ messages in thread
* Re: [PATCH] mm: change size_t to unsigned int for cma_alloc
  2021-03-29 18:25 [PATCH] mm: change size_t to unsigned int for cma_alloc Minchan Kim
@ 2021-03-29 18:44 ` Matthew Wilcox
  2021-03-29 20:12   ` Minchan Kim
  0 siblings, 1 reply; 6+ messages in thread
From: Matthew Wilcox @ 2021-03-29 18:44 UTC (permalink / raw)
  To: Minchan Kim; +Cc: Andrew Morton, linux-mm, LKML

On Mon, Mar 29, 2021 at 11:25:53AM -0700, Minchan Kim wrote:
> size_t in cma_alloc is confusing since it makes people think
> it's byte count, not pages. Fix it.

i think it has to be unsigned long.

67a2e213e7e937c41c52ab5bc46bf3f4de469f6e

> Link: https://lore.kernel.org/linux-mm/20210324043434.GP1719932@casper.infradead.org/
> Cc: Matthew Wilcox <willy@infradead.org>
> Signed-off-by: Minchan Kim <minchan@kernel.org>
> ---
>  include/linux/cma.h | 2 +-
>  mm/cma.c            | 8 ++++----
>  2 files changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/include/linux/cma.h b/include/linux/cma.h
> index 217999c8a762..a873edc20ca2 100644
> --- a/include/linux/cma.h
> +++ b/include/linux/cma.h
> @@ -44,7 +44,7 @@ extern int cma_init_reserved_mem(phys_addr_t base, phys_addr_t size,
>  					unsigned int order_per_bit,
>  					const char *name,
>  					struct cma **res_cma);
> -extern struct page *cma_alloc(struct cma *cma, size_t count, unsigned int align,
> +extern struct page *cma_alloc(struct cma *cma, unsigned int count, unsigned int align,
>  			      bool no_warn);
>  extern bool cma_release(struct cma *cma, const struct page *pages, unsigned int count);
>  
> diff --git a/mm/cma.c b/mm/cma.c
> index 08c45157911a..24dc01e26d45 100644
> --- a/mm/cma.c
> +++ b/mm/cma.c
> @@ -423,21 +423,21 @@ static inline void cma_debug_show_areas(struct cma *cma) { }
>   * This function allocates part of contiguous memory on specific
>   * contiguous memory area.
>   */
> -struct page *cma_alloc(struct cma *cma, size_t count, unsigned int align,
> +struct page *cma_alloc(struct cma *cma, unsigned int count, unsigned int align,
>  		       bool no_warn)
>  {
>  	unsigned long mask, offset;
>  	unsigned long pfn = -1;
>  	unsigned long start = 0;
>  	unsigned long bitmap_maxno, bitmap_no, bitmap_count;
> -	size_t i;
> +	unsigned int i;
>  	struct page *page = NULL;
>  	int ret = -ENOMEM;
>  
>  	if (!cma || !cma->count || !cma->bitmap)
>  		goto out;
>  
> -	pr_debug("%s(cma %p, count %zu, align %d)\n", __func__, (void *)cma,
> +	pr_debug("%s(cma %p, count %u, align %d)\n", __func__, (void *)cma,
>  		 count, align);
>  
>  	if (!count)
> @@ -500,7 +500,7 @@ struct page *cma_alloc(struct cma *cma, size_t count, unsigned int align,
>  	}
>  
>  	if (ret && !no_warn) {
> -		pr_err_ratelimited("%s: %s: alloc failed, req-size: %zu pages, ret: %d\n",
> +		pr_err_ratelimited("%s: %s: alloc failed, req-size: %u pages, ret: %d\n",
>  				   __func__, cma->name, count, ret);
>  		cma_debug_show_areas(cma);
>  	}
> -- 
> 2.31.0.291.g576ba9dcdaf-goog
> 
> 

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH] mm: change size_t to unsigned int for cma_alloc
  2021-03-29 18:44 ` Matthew Wilcox
@ 2021-03-29 20:12   ` Minchan Kim
  2021-03-30  7:58     ` David Hildenbrand
  0 siblings, 1 reply; 6+ messages in thread
From: Minchan Kim @ 2021-03-29 20:12 UTC (permalink / raw)
  To: Matthew Wilcox; +Cc: Andrew Morton, linux-mm, LKML, david

On Mon, Mar 29, 2021 at 07:44:31PM +0100, Matthew Wilcox wrote:
> On Mon, Mar 29, 2021 at 11:25:53AM -0700, Minchan Kim wrote:
> > size_t in cma_alloc is confusing since it makes people think
> > it's byte count, not pages. Fix it.
> 
> i think it has to be unsigned long.
> 
> 67a2e213e7e937c41c52ab5bc46bf3f4de469f6e

Thanks for the pinter. I wanted to have the smallest change.
The commit leads me to change cma_release, trace_cma_alloc,
cma_clear_bitmap and front_contig_range as well.(Not sure
we have more. Will check).

Ccing david@redhat.com for upcoming changing free_contig_range.

> 
> > Link: https://lore.kernel.org/linux-mm/20210324043434.GP1719932@casper.infradead.org/
> > Cc: Matthew Wilcox <willy@infradead.org>
> > Signed-off-by: Minchan Kim <minchan@kernel.org>
> > ---
> >  include/linux/cma.h | 2 +-
> >  mm/cma.c            | 8 ++++----
> >  2 files changed, 5 insertions(+), 5 deletions(-)
> > 
> > diff --git a/include/linux/cma.h b/include/linux/cma.h
> > index 217999c8a762..a873edc20ca2 100644
> > --- a/include/linux/cma.h
> > +++ b/include/linux/cma.h
> > @@ -44,7 +44,7 @@ extern int cma_init_reserved_mem(phys_addr_t base, phys_addr_t size,
> >  					unsigned int order_per_bit,
> >  					const char *name,
> >  					struct cma **res_cma);
> > -extern struct page *cma_alloc(struct cma *cma, size_t count, unsigned int align,
> > +extern struct page *cma_alloc(struct cma *cma, unsigned int count, unsigned int align,
> >  			      bool no_warn);
> >  extern bool cma_release(struct cma *cma, const struct page *pages, unsigned int count);
> >  
> > diff --git a/mm/cma.c b/mm/cma.c
> > index 08c45157911a..24dc01e26d45 100644
> > --- a/mm/cma.c
> > +++ b/mm/cma.c
> > @@ -423,21 +423,21 @@ static inline void cma_debug_show_areas(struct cma *cma) { }
> >   * This function allocates part of contiguous memory on specific
> >   * contiguous memory area.
> >   */
> > -struct page *cma_alloc(struct cma *cma, size_t count, unsigned int align,
> > +struct page *cma_alloc(struct cma *cma, unsigned int count, unsigned int align,
> >  		       bool no_warn)
> >  {
> >  	unsigned long mask, offset;
> >  	unsigned long pfn = -1;
> >  	unsigned long start = 0;
> >  	unsigned long bitmap_maxno, bitmap_no, bitmap_count;
> > -	size_t i;
> > +	unsigned int i;
> >  	struct page *page = NULL;
> >  	int ret = -ENOMEM;
> >  
> >  	if (!cma || !cma->count || !cma->bitmap)
> >  		goto out;
> >  
> > -	pr_debug("%s(cma %p, count %zu, align %d)\n", __func__, (void *)cma,
> > +	pr_debug("%s(cma %p, count %u, align %d)\n", __func__, (void *)cma,
> >  		 count, align);
> >  
> >  	if (!count)
> > @@ -500,7 +500,7 @@ struct page *cma_alloc(struct cma *cma, size_t count, unsigned int align,
> >  	}
> >  
> >  	if (ret && !no_warn) {
> > -		pr_err_ratelimited("%s: %s: alloc failed, req-size: %zu pages, ret: %d\n",
> > +		pr_err_ratelimited("%s: %s: alloc failed, req-size: %u pages, ret: %d\n",
> >  				   __func__, cma->name, count, ret);
> >  		cma_debug_show_areas(cma);
> >  	}
> > -- 
> > 2.31.0.291.g576ba9dcdaf-goog
> > 
> > 

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH] mm: change size_t to unsigned int for cma_alloc
  2021-03-29 20:12   ` Minchan Kim
@ 2021-03-30  7:58     ` David Hildenbrand
  2021-03-30 15:00       ` Minchan Kim
  0 siblings, 1 reply; 6+ messages in thread
From: David Hildenbrand @ 2021-03-30  7:58 UTC (permalink / raw)
  To: Minchan Kim, Matthew Wilcox; +Cc: Andrew Morton, linux-mm, LKML

On 29.03.21 22:12, Minchan Kim wrote:
> On Mon, Mar 29, 2021 at 07:44:31PM +0100, Matthew Wilcox wrote:
>> On Mon, Mar 29, 2021 at 11:25:53AM -0700, Minchan Kim wrote:
>>> size_t in cma_alloc is confusing since it makes people think
>>> it's byte count, not pages. Fix it.
>>
>> i think it has to be unsigned long.
>>
>> 67a2e213e7e937c41c52ab5bc46bf3f4de469f6e

Right.

Fortunately, we don't have such large allocations yet via 
CMA/alloc_contig_range

> 
> Thanks for the pinter. I wanted to have the smallest change.
> The commit leads me to change cma_release, trace_cma_alloc,
> cma_clear_bitmap and front_contig_range as well.(Not sure
> we have more. Will check).
> 
> Ccing david@redhat.com for upcoming changing free_contig_range.

While at it, we might want to convert free_contig_range() to eat
"unsigned long start, unsigned long end" like alloc_contig_range(), 
instead of "unsigned long pfn, unsigned int nr_pages" like 
alloc_contig_pages() ...

-- 
Thanks,

David / dhildenb


^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH] mm: change size_t to unsigned int for cma_alloc
  2021-03-30  7:58     ` David Hildenbrand
@ 2021-03-30 15:00       ` Minchan Kim
  2021-03-30 15:05         ` David Hildenbrand
  0 siblings, 1 reply; 6+ messages in thread
From: Minchan Kim @ 2021-03-30 15:00 UTC (permalink / raw)
  To: David Hildenbrand; +Cc: Matthew Wilcox, Andrew Morton, linux-mm, LKML

On Tue, Mar 30, 2021 at 09:58:37AM +0200, David Hildenbrand wrote:
> On 29.03.21 22:12, Minchan Kim wrote:
> > On Mon, Mar 29, 2021 at 07:44:31PM +0100, Matthew Wilcox wrote:
> > > On Mon, Mar 29, 2021 at 11:25:53AM -0700, Minchan Kim wrote:
> > > > size_t in cma_alloc is confusing since it makes people think
> > > > it's byte count, not pages. Fix it.
> > > 
> > > i think it has to be unsigned long.
> > > 
> > > 67a2e213e7e937c41c52ab5bc46bf3f4de469f6e
> 
> Right.
> 
> Fortunately, we don't have such large allocations yet via
> CMA/alloc_contig_range
> 
> > 
> > Thanks for the pinter. I wanted to have the smallest change.
> > The commit leads me to change cma_release, trace_cma_alloc,
> > cma_clear_bitmap and front_contig_range as well.(Not sure
> > we have more. Will check).
> > 
> > Ccing david@redhat.com for upcoming changing free_contig_range.
> 
> While at it, we might want to convert free_contig_range() to eat
> "unsigned long start, unsigned long end" like alloc_contig_range(), instead
> of "unsigned long pfn, unsigned int nr_pages" like alloc_contig_pages() ...

Well, I personllay tempted to change alloc_contig_range, not
free_contig_range because base_pfn with nr_pages was more
straightforward than base_pfn and end_pfn in that we don't
need to tell whether end_pfn is inclusive or exclusive.

When I look at callers of [alloc|free]_contig_range, many of them
already have used nr_pages based approach rather than start_pfn,
end_pfn. If your suggestion come from that "it's *range* API",
I'd like to rename it with "alloc_contig_pages|free_contig_pages".

Since it's beyond the goal of this patch and might be controversial, 
I will not deal with it in this patch.

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH] mm: change size_t to unsigned int for cma_alloc
  2021-03-30 15:00       ` Minchan Kim
@ 2021-03-30 15:05         ` David Hildenbrand
  0 siblings, 0 replies; 6+ messages in thread
From: David Hildenbrand @ 2021-03-30 15:05 UTC (permalink / raw)
  To: Minchan Kim; +Cc: Matthew Wilcox, Andrew Morton, linux-mm, LKML

On 30.03.21 17:00, Minchan Kim wrote:
> On Tue, Mar 30, 2021 at 09:58:37AM +0200, David Hildenbrand wrote:
>> On 29.03.21 22:12, Minchan Kim wrote:
>>> On Mon, Mar 29, 2021 at 07:44:31PM +0100, Matthew Wilcox wrote:
>>>> On Mon, Mar 29, 2021 at 11:25:53AM -0700, Minchan Kim wrote:
>>>>> size_t in cma_alloc is confusing since it makes people think
>>>>> it's byte count, not pages. Fix it.
>>>>
>>>> i think it has to be unsigned long.
>>>>
>>>> 67a2e213e7e937c41c52ab5bc46bf3f4de469f6e
>>
>> Right.
>>
>> Fortunately, we don't have such large allocations yet via
>> CMA/alloc_contig_range
>>
>>>
>>> Thanks for the pinter. I wanted to have the smallest change.
>>> The commit leads me to change cma_release, trace_cma_alloc,
>>> cma_clear_bitmap and front_contig_range as well.(Not sure
>>> we have more. Will check).
>>>
>>> Ccing david@redhat.com for upcoming changing free_contig_range.
>>
>> While at it, we might want to convert free_contig_range() to eat
>> "unsigned long start, unsigned long end" like alloc_contig_range(), instead
>> of "unsigned long pfn, unsigned int nr_pages" like alloc_contig_pages() ...
> 
> Well, I personllay tempted to change alloc_contig_range, not
> free_contig_range because base_pfn with nr_pages was more
> straightforward than base_pfn and end_pfn in that we don't
> need to tell whether end_pfn is inclusive or exclusive.
> 

That's right.

> When I look at callers of [alloc|free]_contig_range, many of them
> already have used nr_pages based approach rather than start_pfn,
> end_pfn. If your suggestion come from that "it's *range* API",

Right you are, teaching alloc_contig_range() to eat "nr_pages" might 
actually be even better and more consistent.

> I'd like to rename it with "alloc_contig_pages|free_contig_pages".

alloc_contig_pages is just a wrapper for alloc_contig_range(), so 
free_contig_range() is a better fit; OTOH, having both would also 
somehow make sense.

> 
> Since it's beyond the goal of this patch and might be controversial,
> I will not deal with it in this patch.

Sure, but feel free to send a patch to make that consistent. It's been 
bugging me already (having to always remember if to pass in nr_pages or 
end).

-- 
Thanks,

David / dhildenb


^ permalink raw reply	[flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-03-30 15:05 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-03-29 18:25 [PATCH] mm: change size_t to unsigned int for cma_alloc Minchan Kim
2021-03-29 18:44 ` Matthew Wilcox
2021-03-29 20:12   ` Minchan Kim
2021-03-30  7:58     ` David Hildenbrand
2021-03-30 15:00       ` Minchan Kim
2021-03-30 15:05         ` David Hildenbrand

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.