Re: [PATCH] tree wide: Use kvfree() than conditional kfree()/vfree()

[David Rientjes]

On Mon, 9 Nov 2015, Tetsuo Handa wrote:

> There are many locations that do
> 
>   if (memory_was_allocated_by_vmalloc)
> vfree(ptr);
>   else
> kfree(ptr);
> 
> but kvfree() can handle both kmalloc()ed memory and vmalloc()ed memory
> using is_vmalloc_addr(). Unless callers have special reasons, we can
> replace this branch with kvfree(). Please check and reply if you found
> problems.
> 
> Signed-off-by: Tetsuo Handa 
> Acked-by: Michal Hocko 
> Cc: Russell King  # arm
> Cc:  # apei
> Cc:  # drbd
> Cc:  # mspec
> Cc:  # drm
> Cc: Oleg Drokin  # lustre
> Cc: Andreas Dilger  # lustre
> Cc:  # coda
> Cc:  # jffs2
> Cc: Jan Kara  # udf
> Cc:  # xattr
> Cc:  # ipc + mm
> Cc:  # ipv4

Acked-by: David Rientjes 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@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] tree wide: Use kvfree() than conditional kfree()/vfree()

[David Rientjes]

On Mon, 9 Nov 2015, Tetsuo Handa wrote:

> There are many locations that do
> 
>   if (memory_was_allocated_by_vmalloc)
> vfree(ptr);
>   else
> kfree(ptr);
> 
> but kvfree() can handle both kmalloc()ed memory and vmalloc()ed memory
> using is_vmalloc_addr(). Unless callers have special reasons, we can
> replace this branch with kvfree(). Please check and reply if you found
> problems.
> 
> Signed-off-by: Tetsuo Handa 
> Acked-by: Michal Hocko 
> Cc: Russell King  # arm
> Cc:  # apei
> Cc:  # drbd
> Cc:  # mspec
> Cc:  # drm
> Cc: Oleg Drokin  # lustre
> Cc: Andreas Dilger  # lustre
> Cc:  # coda
> Cc:  # jffs2
> Cc: Jan Kara  # udf
> Cc:  # xattr
> Cc:  # ipc + mm
> Cc:  # ipv4

Acked-by: David Rientjes 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@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: [DRBD-user] [PATCH] tree wide: Use kvfree() than conditional kfree()/vfree()

[Lars Ellenberg]

On Mon, Nov 09, 2015 at 08:56:10PM +0900, Tetsuo Handa wrote:
> There are many locations that do
> 
>   if (memory_was_allocated_by_vmalloc)
> vfree(ptr);
>   else
> kfree(ptr);
> 
> but kvfree() can handle both kmalloc()ed memory and vmalloc()ed memory
> using is_vmalloc_addr(). Unless callers have special reasons, we can
> replace this branch with kvfree(). Please check and reply if you found
> problems.

For the DRBD part:

>  drivers/block/drbd/drbd_bitmap.c   | 26 +
>  drivers/block/drbd/drbd_int.h  |  3 --

> diff --git a/drivers/block/drbd/drbd_bitmap.c 
> b/drivers/block/drbd/drbd_bitmap.c
> index 9462d27..2daaafb 100644
> --- a/drivers/block/drbd/drbd_bitmap.c
> +++ b/drivers/block/drbd/drbd_bitmap.c
> @@ -364,12 +364,9 @@ static void bm_free_pages(struct page **pages, unsigned 
> long number)
>   }
>  }
>  
> -static void bm_vk_free(void *ptr, int v)
> +static inline void bm_vk_free(void *ptr)

Maybe drop this inline completely ...

>  {
> - if (v)
> - vfree(ptr);
> - else
> - kfree(ptr);
> + kvfree(ptr);
>  }

... and just call kvfree() directly below?

> - bm_vk_free(new_pages, vmalloced);
> + bm_vk_free(new_pages);

  + kvfree(new_pages);
  ...


Other than that: looks good and harmless enough.
Thanks,

Lars

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@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: [DRBD-user] [PATCH] tree wide: Use kvfree() than conditional kfree()/vfree()

[Lars Ellenberg]

On Mon, Nov 09, 2015 at 08:56:10PM +0900, Tetsuo Handa wrote:
> There are many locations that do
> 
>   if (memory_was_allocated_by_vmalloc)
> vfree(ptr);
>   else
> kfree(ptr);
> 
> but kvfree() can handle both kmalloc()ed memory and vmalloc()ed memory
> using is_vmalloc_addr(). Unless callers have special reasons, we can
> replace this branch with kvfree(). Please check and reply if you found
> problems.

For the DRBD part:

>  drivers/block/drbd/drbd_bitmap.c   | 26 +
>  drivers/block/drbd/drbd_int.h  |  3 --

> diff --git a/drivers/block/drbd/drbd_bitmap.c 
> b/drivers/block/drbd/drbd_bitmap.c
> index 9462d27..2daaafb 100644
> --- a/drivers/block/drbd/drbd_bitmap.c
> +++ b/drivers/block/drbd/drbd_bitmap.c
> @@ -364,12 +364,9 @@ static void bm_free_pages(struct page **pages, unsigned 
> long number)
>   }
>  }
>  
> -static void bm_vk_free(void *ptr, int v)
> +static inline void bm_vk_free(void *ptr)

Maybe drop this inline completely ...

>  {
> - if (v)
> - vfree(ptr);
> - else
> - kfree(ptr);
> + kvfree(ptr);
>  }

... and just call kvfree() directly below?

> - bm_vk_free(new_pages, vmalloced);
> + bm_vk_free(new_pages);

  + kvfree(new_pages);
  ...


Other than that: looks good and harmless enough.
Thanks,

Lars

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@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] tree wide: Use kvfree() than conditional kfree()/vfree()

[Luck, Tony]

> ACK for the ACPI changes (and CCing Tony and Boris for the heads-up as they
> are way more famailiar with the APEI code than I am).

Sure. If kvfree() really is smart enough to figure it out then there
it no point in the if (blah) kfree() else vfree().

The drivers/acpi/apei/erst.c code isn't doing anything subtle or magic here.

-Tony

Re: [PATCH] tree wide: Use kvfree() than conditional kfree()/vfree()

[Rafael J. Wysocki]

On Monday, November 09, 2015 08:56:10 PM Tetsuo Handa wrote:
> There are many locations that do
> 
>   if (memory_was_allocated_by_vmalloc)
> vfree(ptr);
>   else
> kfree(ptr);
> 
> but kvfree() can handle both kmalloc()ed memory and vmalloc()ed memory
> using is_vmalloc_addr(). Unless callers have special reasons, we can
> replace this branch with kvfree(). Please check and reply if you found
> problems.

ACK for the ACPI changes (and CCing Tony and Boris for the heads-up as they
are way more famailiar with the APEI code than I am).

Thanks,
Rafael

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@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] tree wide: Use kvfree() than conditional kfree()/vfree()

[Dilger, Andreas]

On 2015/11/09, 04:56, "Tetsuo Handa" 
wrote:

>There are many locations that do
>
>  if (memory_was_allocated_by_vmalloc)
>vfree(ptr);
>  else
>kfree(ptr);
>
>but kvfree() can handle both kmalloc()ed memory and vmalloc()ed memory
>using is_vmalloc_addr(). Unless callers have special reasons, we can
>replace this branch with kvfree(). Please check and reply if you found
>problems.
>
>Signed-off-by: Tetsuo Handa 
>Acked-by: Michal Hocko 

For Lustre part:
Reviewed-by: Andreas Dilger 

Cheers, Andreas
-- 
Andreas Dilger

Lustre Principal Engineer
Intel High Performance Data Division


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@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] tree wide: Use kvfree() than conditional kfree()/vfree()

[David Miller]

From: Jan Kara 
Date: Mon, 9 Nov 2015 13:11:26 +0100

> You can add
> 
> Acked-by: Jan Kara 
> 
> for the UDF and fs/xattr.c parts.

Please do not quote and entire large patch just to give an ACK.

Just quote the minimum necessary context, which is usually just
the commit message.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@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] tree wide: Use kvfree() than conditional kfree()/vfree()

[Russell King - ARM Linux]

On Mon, Nov 09, 2015 at 08:56:10PM +0900, Tetsuo Handa wrote:
> There are many locations that do
> 
>   if (memory_was_allocated_by_vmalloc)
> vfree(ptr);
>   else
> kfree(ptr);
> 
> but kvfree() can handle both kmalloc()ed memory and vmalloc()ed memory
> using is_vmalloc_addr(). Unless callers have special reasons, we can
> replace this branch with kvfree(). Please check and reply if you found
> problems.
> 
> Signed-off-by: Tetsuo Handa 
> Acked-by: Michal Hocko 
> Cc: Russell King  # arm

Acked-by: Russell King 

In so far as this ARM specific change looks reasonable.  Thanks.

> diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c
> index e62400e..492bf3e 100644
> --- a/arch/arm/mm/dma-mapping.c
> +++ b/arch/arm/mm/dma-mapping.c
> @@ -1200,10 +1200,7 @@ error:
>   while (i--)
>   if (pages[i])
>   __free_pages(pages[i], 0);
> - if (array_size <= PAGE_SIZE)
> - kfree(pages);
> - else
> - vfree(pages);
> + kvfree(pages);
>   return NULL;
>  }
>  
> @@ -1211,7 +1208,6 @@ static int __iommu_free_buffer(struct device *dev, 
> struct page **pages,
>  size_t size, struct dma_attrs *attrs)
>  {
>   int count = size >> PAGE_SHIFT;
> - int array_size = count * sizeof(struct page *);
>   int i;
>  
>   if (dma_get_attr(DMA_ATTR_FORCE_CONTIGUOUS, attrs)) {
> @@ -1222,10 +1218,7 @@ static int __iommu_free_buffer(struct device *dev, 
> struct page **pages,
>   __free_pages(pages[i], 0);
>   }
>  
> - if (array_size <= PAGE_SIZE)
> - kfree(pages);
> - else
> - vfree(pages);
> + kvfree(pages);
>   return 0;
>  }
>  

-- 
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@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] tree wide: Use kvfree() than conditional kfree()/vfree()

[Jan Kara]

On Mon 09-11-15 20:56:10, Tetsuo Handa wrote:
> There are many locations that do
> 
>   if (memory_was_allocated_by_vmalloc)
> vfree(ptr);
>   else
> kfree(ptr);
> 
> but kvfree() can handle both kmalloc()ed memory and vmalloc()ed memory
> using is_vmalloc_addr(). Unless callers have special reasons, we can
> replace this branch with kvfree(). Please check and reply if you found
> problems.
> 
> Signed-off-by: Tetsuo Handa 
> Acked-by: Michal Hocko 

You can add

Acked-by: Jan Kara 

for the UDF and fs/xattr.c parts.

Honza

> Cc: Russell King  # arm
> Cc:  # apei
> Cc:  # drbd
> Cc:  # mspec
> Cc:  # drm
> Cc: Oleg Drokin  # lustre
> Cc: Andreas Dilger  # lustre
> Cc:  # coda
> Cc:  # jffs2
> Cc: Jan Kara  # udf
> Cc:  # xattr
> Cc:  # ipc + mm
> Cc:  # ipv4
> ---
>  arch/arm/mm/dma-mapping.c  | 11 ++--
>  drivers/acpi/apei/erst.c   |  6 ++--
>  drivers/block/drbd/drbd_bitmap.c   | 26 +
>  drivers/block/drbd/drbd_int.h  |  3 --
>  drivers/char/mspec.c   | 15 ++
>  drivers/gpu/drm/drm_hashtab.c  |  5 +---
>  .../lustre/include/linux/libcfs/libcfs_private.h   |  8 ++
>  fs/coda/coda_linux.h   |  3 +-
>  fs/jffs2/build.c   |  8 ++
>  fs/jffs2/fs.c  |  5 +---
>  fs/jffs2/super.c   |  5 +---
>  fs/udf/super.c |  7 +
>  fs/xattr.c | 33 
> ++
>  ipc/sem.c  |  2 +-
>  ipc/util.c | 11 ++--
>  ipc/util.h |  2 +-
>  mm/percpu.c| 18 +---
>  mm/vmalloc.c   |  5 +---
>  net/ipv4/fib_trie.c|  4 +--
>  19 files changed, 46 insertions(+), 131 deletions(-)
> 
> diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c
> index e62400e..492bf3e 100644
> --- a/arch/arm/mm/dma-mapping.c
> +++ b/arch/arm/mm/dma-mapping.c
> @@ -1200,10 +1200,7 @@ error:
>   while (i--)
>   if (pages[i])
>   __free_pages(pages[i], 0);
> - if (array_size <= PAGE_SIZE)
> - kfree(pages);
> - else
> - vfree(pages);
> + kvfree(pages);
>   return NULL;
>  }
>  
> @@ -1211,7 +1208,6 @@ static int __iommu_free_buffer(struct device *dev, 
> struct page **pages,
>  size_t size, struct dma_attrs *attrs)
>  {
>   int count = size >> PAGE_SHIFT;
> - int array_size = count * sizeof(struct page *);
>   int i;
>  
>   if (dma_get_attr(DMA_ATTR_FORCE_CONTIGUOUS, attrs)) {
> @@ -1222,10 +1218,7 @@ static int __iommu_free_buffer(struct device *dev, 
> struct page **pages,
>   __free_pages(pages[i], 0);
>   }
>  
> - if (array_size <= PAGE_SIZE)
> - kfree(pages);
> - else
> - vfree(pages);
> + kvfree(pages);
>   return 0;
>  }
>  
> diff --git a/drivers/acpi/apei/erst.c b/drivers/acpi/apei/erst.c
> index 6682c5d..6e6bc10 100644
> --- a/drivers/acpi/apei/erst.c
> +++ b/drivers/acpi/apei/erst.c
> @@ -32,6 +32,7 @@
>  #include 
>  #include 
>  #include 
> +#include  /* kvfree() */
>  #include 
>  
>  #include "apei-internal.h"
> @@ -532,10 +533,7 @@ retry:
>   return -ENOMEM;
>   memcpy(new_entries, entries,
>  erst_record_id_cache.len * sizeof(entries[0]));
> - if (erst_record_id_cache.size < PAGE_SIZE)
> - kfree(entries);
> - else
> - vfree(entries);
> + kvfree(entries);
>   erst_record_id_cache.entries = entries = new_entries;
>   erst_record_id_cache.size = new_size;
>   }
> diff --git a/drivers/block/drbd/drbd_bitmap.c 
> b/drivers/block/drbd/drbd_bitmap.c
> index 9462d27..2daaafb 100644
> --- a/drivers/block/drbd/drbd_bitmap.c
> +++ b/drivers/block/drbd/drbd_bitmap.c
> @@ -364,12 +364,9 @@ static void bm_free_pages(struct page **pages, unsigned 
> long number)
>   }
>  }
>  
> -static void bm_vk_free(void *ptr, int v)
> +static inline void bm_vk_free(void *ptr)
>  {
> - if (v)
> - vfree(ptr);
> - else
> - kfree(ptr);
> + kvfree(ptr);
>  }
>  
>  /*
> @@ -379,7 +376,7 @@ static struct page **bm_realloc_pages(struct drbd_bitmap 
> *b, unsigned long want)
>  {
>   struct page **old_pages = b->bm_pages;
>   struct page **new_pages, *page;
> - unsigned int i, bytes, vmalloced = 0;
> + unsigned int i, bytes;
>   unsigned long have = b->bm_number_of_pages;
>  
> 

[PATCH] tree wide: Use kvfree() than conditional kfree()/vfree()

[Tetsuo Handa]

There are many locations that do

  if (memory_was_allocated_by_vmalloc)
vfree(ptr);
  else
kfree(ptr);

but kvfree() can handle both kmalloc()ed memory and vmalloc()ed memory
using is_vmalloc_addr(). Unless callers have special reasons, we can
replace this branch with kvfree(). Please check and reply if you found
problems.

Signed-off-by: Tetsuo Handa 
Acked-by: Michal Hocko 
Cc: Russell King  # arm
Cc:  # apei
Cc:  # drbd
Cc:  # mspec
Cc:  # drm
Cc: Oleg Drokin  # lustre
Cc: Andreas Dilger  # lustre
Cc:  # coda
Cc:  # jffs2
Cc: Jan Kara  # udf
Cc:  # xattr
Cc:  # ipc + mm
Cc:  # ipv4
---
 arch/arm/mm/dma-mapping.c  | 11 ++--
 drivers/acpi/apei/erst.c   |  6 ++--
 drivers/block/drbd/drbd_bitmap.c   | 26 +
 drivers/block/drbd/drbd_int.h  |  3 --
 drivers/char/mspec.c   | 15 ++
 drivers/gpu/drm/drm_hashtab.c  |  5 +---
 .../lustre/include/linux/libcfs/libcfs_private.h   |  8 ++
 fs/coda/coda_linux.h   |  3 +-
 fs/jffs2/build.c   |  8 ++
 fs/jffs2/fs.c  |  5 +---
 fs/jffs2/super.c   |  5 +---
 fs/udf/super.c |  7 +
 fs/xattr.c | 33 ++
 ipc/sem.c  |  2 +-
 ipc/util.c | 11 ++--
 ipc/util.h |  2 +-
 mm/percpu.c| 18 +---
 mm/vmalloc.c   |  5 +---
 net/ipv4/fib_trie.c|  4 +--
 19 files changed, 46 insertions(+), 131 deletions(-)

diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c
index e62400e..492bf3e 100644
--- a/arch/arm/mm/dma-mapping.c
+++ b/arch/arm/mm/dma-mapping.c
@@ -1200,10 +1200,7 @@ error:
while (i--)
if (pages[i])
__free_pages(pages[i], 0);
-   if (array_size <= PAGE_SIZE)
-   kfree(pages);
-   else
-   vfree(pages);
+   kvfree(pages);
return NULL;
 }
 
@@ -1211,7 +1208,6 @@ static int __iommu_free_buffer(struct device *dev, struct 
page **pages,
   size_t size, struct dma_attrs *attrs)
 {
int count = size >> PAGE_SHIFT;
-   int array_size = count * sizeof(struct page *);
int i;
 
if (dma_get_attr(DMA_ATTR_FORCE_CONTIGUOUS, attrs)) {
@@ -1222,10 +1218,7 @@ static int __iommu_free_buffer(struct device *dev, 
struct page **pages,
__free_pages(pages[i], 0);
}
 
-   if (array_size <= PAGE_SIZE)
-   kfree(pages);
-   else
-   vfree(pages);
+   kvfree(pages);
return 0;
 }
 
diff --git a/drivers/acpi/apei/erst.c b/drivers/acpi/apei/erst.c
index 6682c5d..6e6bc10 100644
--- a/drivers/acpi/apei/erst.c
+++ b/drivers/acpi/apei/erst.c
@@ -32,6 +32,7 @@
 #include 
 #include 
 #include 
+#include  /* kvfree() */
 #include 
 
 #include "apei-internal.h"
@@ -532,10 +533,7 @@ retry:
return -ENOMEM;
memcpy(new_entries, entries,
   erst_record_id_cache.len * sizeof(entries[0]));
-   if (erst_record_id_cache.size < PAGE_SIZE)
-   kfree(entries);
-   else
-   vfree(entries);
+   kvfree(entries);
erst_record_id_cache.entries = entries = new_entries;
erst_record_id_cache.size = new_size;
}
diff --git a/drivers/block/drbd/drbd_bitmap.c b/drivers/block/drbd/drbd_bitmap.c
index 9462d27..2daaafb 100644
--- a/drivers/block/drbd/drbd_bitmap.c
+++ b/drivers/block/drbd/drbd_bitmap.c
@@ -364,12 +364,9 @@ static void bm_free_pages(struct page **pages, unsigned 
long number)
}
 }
 
-static void bm_vk_free(void *ptr, int v)
+static inline void bm_vk_free(void *ptr)
 {
-   if (v)
-   vfree(ptr);
-   else
-   kfree(ptr);
+   kvfree(ptr);
 }
 
 /*
@@ -379,7 +376,7 @@ static struct page **bm_realloc_pages(struct drbd_bitmap 
*b, unsigned long want)
 {
struct page **old_pages = b->bm_pages;
struct page **new_pages, *page;
-   unsigned int i, bytes, vmalloced = 0;
+   unsigned int i, bytes;
unsigned long have = b->bm_number_of_pages;
 
BUG_ON(have == 0 && old_pages != NULL);
@@ -401,7 +398,6 @@ static struct page **bm_realloc_pages(struct drbd_bitmap 
*b, unsigned long want)
PAGE_KERNEL);
if (!new_pages)
return NULL;
-   vmalloced = 1;
}
 
if (want >= have) {
@@ -411,7 +407,7 @@ static struct page 

Re: [PATCH] tree wide: Use kvfree() than conditional kfree()/vfree()

[Michal Hocko]

On Sat 07-11-15 20:44:25, Tetsuo Handa wrote:
> There are many locations that do
> 
>   if (memory_was_allocated_by_vmalloc)
> vfree(ptr);
>   else
> kfree(ptr);
> 
> but kvfree() can handle both kmalloc()ed memory and vmalloc()ed memory
> using is_vmalloc_addr(). Unless callers have special reasons, we can
> replace this branch with kvfree().

Yes I agree this is preferable. Doing size assumptions or convolute the
code with additional flags (e.g. drbd) is very error prone and harder
to follow. Using kvfree in those places is much more easier to read
and understand while it shouldn't make the code more heavy because
is_vmalloc_addr is a simple check.

> Signed-off-by: Tetsuo Handa 

I am not familiar with most of the patched code but it seems doing the
right thing AFAICT. So for what-ever this is worth
Acked-by: Michal Hocko 

I think you should CC all the relevant maintainers though.

> ---
>  arch/arm/mm/dma-mapping.c  | 11 ++--
>  drivers/acpi/apei/erst.c   |  6 ++--
>  drivers/block/drbd/drbd_bitmap.c   | 26 +
>  drivers/block/drbd/drbd_int.h  |  3 --
>  drivers/char/mspec.c   | 15 ++
>  drivers/gpu/drm/drm_hashtab.c  |  5 +---
>  .../lustre/include/linux/libcfs/libcfs_private.h   |  8 ++
>  fs/coda/coda_linux.h   |  3 +-
>  fs/jffs2/build.c   |  8 ++
>  fs/jffs2/fs.c  |  5 +---
>  fs/jffs2/super.c   |  5 +---
>  fs/udf/super.c |  7 +
>  fs/xattr.c | 33 
> ++
>  ipc/sem.c  |  2 +-
>  ipc/util.c |  8 ++
>  ipc/util.h |  2 +-
>  mm/percpu.c| 18 +---
>  mm/vmalloc.c   |  5 +---
>  net/ipv4/fib_trie.c|  4 +--
>  19 files changed, 45 insertions(+), 129 deletions(-)
> 
> diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c
> index e62400e..492bf3e 100644
> --- a/arch/arm/mm/dma-mapping.c
> +++ b/arch/arm/mm/dma-mapping.c
> @@ -1200,10 +1200,7 @@ error:
>   while (i--)
>   if (pages[i])
>   __free_pages(pages[i], 0);
> - if (array_size <= PAGE_SIZE)
> - kfree(pages);
> - else
> - vfree(pages);
> + kvfree(pages);
>   return NULL;
>  }
>  
> @@ -1211,7 +1208,6 @@ static int __iommu_free_buffer(struct device *dev, 
> struct page **pages,
>  size_t size, struct dma_attrs *attrs)
>  {
>   int count = size >> PAGE_SHIFT;
> - int array_size = count * sizeof(struct page *);
>   int i;
>  
>   if (dma_get_attr(DMA_ATTR_FORCE_CONTIGUOUS, attrs)) {
> @@ -1222,10 +1218,7 @@ static int __iommu_free_buffer(struct device *dev, 
> struct page **pages,
>   __free_pages(pages[i], 0);
>   }
>  
> - if (array_size <= PAGE_SIZE)
> - kfree(pages);
> - else
> - vfree(pages);
> + kvfree(pages);
>   return 0;
>  }
>  
> diff --git a/drivers/acpi/apei/erst.c b/drivers/acpi/apei/erst.c
> index 6682c5d..6e6bc10 100644
> --- a/drivers/acpi/apei/erst.c
> +++ b/drivers/acpi/apei/erst.c
> @@ -32,6 +32,7 @@
>  #include 
>  #include 
>  #include 
> +#include  /* kvfree() */
>  #include 
>  
>  #include "apei-internal.h"
> @@ -532,10 +533,7 @@ retry:
>   return -ENOMEM;
>   memcpy(new_entries, entries,
>  erst_record_id_cache.len * sizeof(entries[0]));
> - if (erst_record_id_cache.size < PAGE_SIZE)
> - kfree(entries);
> - else
> - vfree(entries);
> + kvfree(entries);
>   erst_record_id_cache.entries = entries = new_entries;
>   erst_record_id_cache.size = new_size;
>   }
> diff --git a/drivers/block/drbd/drbd_bitmap.c 
> b/drivers/block/drbd/drbd_bitmap.c
> index 9462d27..a090fb7 100644
> --- a/drivers/block/drbd/drbd_bitmap.c
> +++ b/drivers/block/drbd/drbd_bitmap.c
> @@ -364,12 +364,9 @@ static void bm_free_pages(struct page **pages, unsigned 
> long number)
>   }
>  }
>  
> -static void bm_vk_free(void *ptr, int v)
> +static void bm_vk_free(void *ptr)
>  {
> - if (v)
> - vfree(ptr);
> - else
> - kfree(ptr);
> + kvfree(ptr);
>  }
>  
>  /*
> @@ -379,7 +376,7 @@ static struct page **bm_realloc_pages(struct drbd_bitmap 
> *b, unsigned long want)
>  {
>   struct page **old_pages = b->bm_pages;
>   struct page **new_pages, *page;
> - unsigned int i, bytes, vmalloced = 0;
> + unsigned int i, bytes;
> 

Re: [PATCH] tree wide: Use kvfree() than conditional kfree()/vfree()

[Rafael J. Wysocki]

On Monday, November 09, 2015 08:56:10 PM Tetsuo Handa wrote:
> There are many locations that do
> 
>   if (memory_was_allocated_by_vmalloc)
> vfree(ptr);
>   else
> kfree(ptr);
> 
> but kvfree() can handle both kmalloc()ed memory and vmalloc()ed memory
> using is_vmalloc_addr(). Unless callers have special reasons, we can
> replace this branch with kvfree(). Please check and reply if you found
> problems.

ACK for the ACPI changes (and CCing Tony and Boris for the heads-up as they
are way more famailiar with the APEI code than I am).

Thanks,
Rafael

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@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] tree wide: Use kvfree() than conditional kfree()/vfree()

[David Miller]

From: Jan Kara 
Date: Mon, 9 Nov 2015 13:11:26 +0100

> You can add
> 
> Acked-by: Jan Kara 
> 
> for the UDF and fs/xattr.c parts.

Please do not quote and entire large patch just to give an ACK.

Just quote the minimum necessary context, which is usually just
the commit message.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@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] tree wide: Use kvfree() than conditional kfree()/vfree()

[Russell King - ARM Linux]

On Mon, Nov 09, 2015 at 08:56:10PM +0900, Tetsuo Handa wrote:
> There are many locations that do
> 
>   if (memory_was_allocated_by_vmalloc)
> vfree(ptr);
>   else
> kfree(ptr);
> 
> but kvfree() can handle both kmalloc()ed memory and vmalloc()ed memory
> using is_vmalloc_addr(). Unless callers have special reasons, we can
> replace this branch with kvfree(). Please check and reply if you found
> problems.
> 
> Signed-off-by: Tetsuo Handa 
> Acked-by: Michal Hocko 
> Cc: Russell King  # arm

Acked-by: Russell King 

In so far as this ARM specific change looks reasonable.  Thanks.

> diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c
> index e62400e..492bf3e 100644
> --- a/arch/arm/mm/dma-mapping.c
> +++ b/arch/arm/mm/dma-mapping.c
> @@ -1200,10 +1200,7 @@ error:
>   while (i--)
>   if (pages[i])
>   __free_pages(pages[i], 0);
> - if (array_size <= PAGE_SIZE)
> - kfree(pages);
> - else
> - vfree(pages);
> + kvfree(pages);
>   return NULL;
>  }
>  
> @@ -1211,7 +1208,6 @@ static int __iommu_free_buffer(struct device *dev, 
> struct page **pages,
>  size_t size, struct dma_attrs *attrs)
>  {
>   int count = size >> PAGE_SHIFT;
> - int array_size = count * sizeof(struct page *);
>   int i;
>  
>   if (dma_get_attr(DMA_ATTR_FORCE_CONTIGUOUS, attrs)) {
> @@ -1222,10 +1218,7 @@ static int __iommu_free_buffer(struct device *dev, 
> struct page **pages,
>   __free_pages(pages[i], 0);
>   }
>  
> - if (array_size <= PAGE_SIZE)
> - kfree(pages);
> - else
> - vfree(pages);
> + kvfree(pages);
>   return 0;
>  }
>  

-- 
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@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] tree wide: Use kvfree() than conditional kfree()/vfree()

[Dilger, Andreas]

On 2015/11/09, 04:56, "Tetsuo Handa" 
wrote:

>There are many locations that do
>
>  if (memory_was_allocated_by_vmalloc)
>vfree(ptr);
>  else
>kfree(ptr);
>
>but kvfree() can handle both kmalloc()ed memory and vmalloc()ed memory
>using is_vmalloc_addr(). Unless callers have special reasons, we can
>replace this branch with kvfree(). Please check and reply if you found
>problems.
>
>Signed-off-by: Tetsuo Handa 
>Acked-by: Michal Hocko 

For Lustre part:
Reviewed-by: Andreas Dilger 

Cheers, Andreas
-- 
Andreas Dilger

Lustre Principal Engineer
Intel High Performance Data Division


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@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] tree wide: Use kvfree() than conditional kfree()/vfree()

[Luck, Tony]

> ACK for the ACPI changes (and CCing Tony and Boris for the heads-up as they
> are way more famailiar with the APEI code than I am).

Sure. If kvfree() really is smart enough to figure it out then there
it no point in the if (blah) kfree() else vfree().

The drivers/acpi/apei/erst.c code isn't doing anything subtle or magic here.

-Tony

Re: [PATCH] tree wide: Use kvfree() than conditional kfree()/vfree()

[Michal Hocko]

On Sat 07-11-15 20:44:25, Tetsuo Handa wrote:
> There are many locations that do
> 
>   if (memory_was_allocated_by_vmalloc)
> vfree(ptr);
>   else
> kfree(ptr);
> 
> but kvfree() can handle both kmalloc()ed memory and vmalloc()ed memory
> using is_vmalloc_addr(). Unless callers have special reasons, we can
> replace this branch with kvfree().

Yes I agree this is preferable. Doing size assumptions or convolute the
code with additional flags (e.g. drbd) is very error prone and harder
to follow. Using kvfree in those places is much more easier to read
and understand while it shouldn't make the code more heavy because
is_vmalloc_addr is a simple check.

> Signed-off-by: Tetsuo Handa 

I am not familiar with most of the patched code but it seems doing the
right thing AFAICT. So for what-ever this is worth
Acked-by: Michal Hocko 

I think you should CC all the relevant maintainers though.

> ---
>  arch/arm/mm/dma-mapping.c  | 11 ++--
>  drivers/acpi/apei/erst.c   |  6 ++--
>  drivers/block/drbd/drbd_bitmap.c   | 26 +
>  drivers/block/drbd/drbd_int.h  |  3 --
>  drivers/char/mspec.c   | 15 ++
>  drivers/gpu/drm/drm_hashtab.c  |  5 +---
>  .../lustre/include/linux/libcfs/libcfs_private.h   |  8 ++
>  fs/coda/coda_linux.h   |  3 +-
>  fs/jffs2/build.c   |  8 ++
>  fs/jffs2/fs.c  |  5 +---
>  fs/jffs2/super.c   |  5 +---
>  fs/udf/super.c |  7 +
>  fs/xattr.c | 33 
> ++
>  ipc/sem.c  |  2 +-
>  ipc/util.c |  8 ++
>  ipc/util.h |  2 +-
>  mm/percpu.c| 18 +---
>  mm/vmalloc.c   |  5 +---
>  net/ipv4/fib_trie.c|  4 +--
>  19 files changed, 45 insertions(+), 129 deletions(-)
> 
> diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c
> index e62400e..492bf3e 100644
> --- a/arch/arm/mm/dma-mapping.c
> +++ b/arch/arm/mm/dma-mapping.c
> @@ -1200,10 +1200,7 @@ error:
>   while (i--)
>   if (pages[i])
>   __free_pages(pages[i], 0);
> - if (array_size <= PAGE_SIZE)
> - kfree(pages);
> - else
> - vfree(pages);
> + kvfree(pages);
>   return NULL;
>  }
>  
> @@ -1211,7 +1208,6 @@ static int __iommu_free_buffer(struct device *dev, 
> struct page **pages,
>  size_t size, struct dma_attrs *attrs)
>  {
>   int count = size >> PAGE_SHIFT;
> - int array_size = count * sizeof(struct page *);
>   int i;
>  
>   if (dma_get_attr(DMA_ATTR_FORCE_CONTIGUOUS, attrs)) {
> @@ -1222,10 +1218,7 @@ static int __iommu_free_buffer(struct device *dev, 
> struct page **pages,
>   __free_pages(pages[i], 0);
>   }
>  
> - if (array_size <= PAGE_SIZE)
> - kfree(pages);
> - else
> - vfree(pages);
> + kvfree(pages);
>   return 0;
>  }
>  
> diff --git a/drivers/acpi/apei/erst.c b/drivers/acpi/apei/erst.c
> index 6682c5d..6e6bc10 100644
> --- a/drivers/acpi/apei/erst.c
> +++ b/drivers/acpi/apei/erst.c
> @@ -32,6 +32,7 @@
>  #include 
>  #include 
>  #include 
> +#include  /* kvfree() */
>  #include 
>  
>  #include "apei-internal.h"
> @@ -532,10 +533,7 @@ retry:
>   return -ENOMEM;
>   memcpy(new_entries, entries,
>  erst_record_id_cache.len * sizeof(entries[0]));
> - if (erst_record_id_cache.size < PAGE_SIZE)
> - kfree(entries);
> - else
> - vfree(entries);
> + kvfree(entries);
>   erst_record_id_cache.entries = entries = new_entries;
>   erst_record_id_cache.size = new_size;
>   }
> diff --git a/drivers/block/drbd/drbd_bitmap.c 
> b/drivers/block/drbd/drbd_bitmap.c
> index 9462d27..a090fb7 100644
> --- a/drivers/block/drbd/drbd_bitmap.c
> +++ b/drivers/block/drbd/drbd_bitmap.c
> @@ -364,12 +364,9 @@ static void bm_free_pages(struct page **pages, unsigned 
> long number)
>   }
>  }
>  
> -static void bm_vk_free(void *ptr, int v)
> +static void bm_vk_free(void *ptr)
>  {
> - if (v)
> - vfree(ptr);
> - else
> - kfree(ptr);
> + kvfree(ptr);
>  }
>  
>  /*
> @@ -379,7 +376,7 @@ static struct page **bm_realloc_pages(struct drbd_bitmap 
> *b, unsigned long want)
>  {
>   struct page **old_pages = b->bm_pages;
>   struct page **new_pages, *page;
> - unsigned int i, 

[PATCH] tree wide: Use kvfree() than conditional kfree()/vfree()

[Tetsuo Handa]

There are many locations that do

  if (memory_was_allocated_by_vmalloc)
vfree(ptr);
  else
kfree(ptr);

but kvfree() can handle both kmalloc()ed memory and vmalloc()ed memory
using is_vmalloc_addr(). Unless callers have special reasons, we can
replace this branch with kvfree(). Please check and reply if you found
problems.

Signed-off-by: Tetsuo Handa 
Acked-by: Michal Hocko 
Cc: Russell King  # arm
Cc:  # apei
Cc:  # drbd
Cc:  # mspec
Cc:  # drm
Cc: Oleg Drokin  # lustre
Cc: Andreas Dilger  # lustre
Cc:  # coda
Cc:  # jffs2
Cc: Jan Kara  # udf
Cc:  # xattr
Cc:  # ipc + mm
Cc:  # ipv4
---
 arch/arm/mm/dma-mapping.c  | 11 ++--
 drivers/acpi/apei/erst.c   |  6 ++--
 drivers/block/drbd/drbd_bitmap.c   | 26 +
 drivers/block/drbd/drbd_int.h  |  3 --
 drivers/char/mspec.c   | 15 ++
 drivers/gpu/drm/drm_hashtab.c  |  5 +---
 .../lustre/include/linux/libcfs/libcfs_private.h   |  8 ++
 fs/coda/coda_linux.h   |  3 +-
 fs/jffs2/build.c   |  8 ++
 fs/jffs2/fs.c  |  5 +---
 fs/jffs2/super.c   |  5 +---
 fs/udf/super.c |  7 +
 fs/xattr.c | 33 ++
 ipc/sem.c  |  2 +-
 ipc/util.c | 11 ++--
 ipc/util.h |  2 +-
 mm/percpu.c| 18 +---
 mm/vmalloc.c   |  5 +---
 net/ipv4/fib_trie.c|  4 +--
 19 files changed, 46 insertions(+), 131 deletions(-)

diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c
index e62400e..492bf3e 100644
--- a/arch/arm/mm/dma-mapping.c
+++ b/arch/arm/mm/dma-mapping.c
@@ -1200,10 +1200,7 @@ error:
while (i--)
if (pages[i])
__free_pages(pages[i], 0);
-   if (array_size <= PAGE_SIZE)
-   kfree(pages);
-   else
-   vfree(pages);
+   kvfree(pages);
return NULL;
 }
 
@@ -1211,7 +1208,6 @@ static int __iommu_free_buffer(struct device *dev, struct 
page **pages,
   size_t size, struct dma_attrs *attrs)
 {
int count = size >> PAGE_SHIFT;
-   int array_size = count * sizeof(struct page *);
int i;
 
if (dma_get_attr(DMA_ATTR_FORCE_CONTIGUOUS, attrs)) {
@@ -1222,10 +1218,7 @@ static int __iommu_free_buffer(struct device *dev, 
struct page **pages,
__free_pages(pages[i], 0);
}
 
-   if (array_size <= PAGE_SIZE)
-   kfree(pages);
-   else
-   vfree(pages);
+   kvfree(pages);
return 0;
 }
 
diff --git a/drivers/acpi/apei/erst.c b/drivers/acpi/apei/erst.c
index 6682c5d..6e6bc10 100644
--- a/drivers/acpi/apei/erst.c
+++ b/drivers/acpi/apei/erst.c
@@ -32,6 +32,7 @@
 #include 
 #include 
 #include 
+#include  /* kvfree() */
 #include 
 
 #include "apei-internal.h"
@@ -532,10 +533,7 @@ retry:
return -ENOMEM;
memcpy(new_entries, entries,
   erst_record_id_cache.len * sizeof(entries[0]));
-   if (erst_record_id_cache.size < PAGE_SIZE)
-   kfree(entries);
-   else
-   vfree(entries);
+   kvfree(entries);
erst_record_id_cache.entries = entries = new_entries;
erst_record_id_cache.size = new_size;
}
diff --git a/drivers/block/drbd/drbd_bitmap.c b/drivers/block/drbd/drbd_bitmap.c
index 9462d27..2daaafb 100644
--- a/drivers/block/drbd/drbd_bitmap.c
+++ b/drivers/block/drbd/drbd_bitmap.c
@@ -364,12 +364,9 @@ static void bm_free_pages(struct page **pages, unsigned 
long number)
}
 }
 
-static void bm_vk_free(void *ptr, int v)
+static inline void bm_vk_free(void *ptr)
 {
-   if (v)
-   vfree(ptr);
-   else
-   kfree(ptr);
+   kvfree(ptr);
 }
 
 /*
@@ -379,7 +376,7 @@ static struct page **bm_realloc_pages(struct drbd_bitmap 
*b, unsigned long want)
 {
struct page **old_pages = b->bm_pages;
struct page **new_pages, *page;
-   unsigned int i, bytes, vmalloced = 0;
+   unsigned int i, bytes;
unsigned long have = b->bm_number_of_pages;
 

Re: [PATCH] tree wide: Use kvfree() than conditional kfree()/vfree()

[Jan Kara]

On Mon 09-11-15 20:56:10, Tetsuo Handa wrote:
> There are many locations that do
> 
>   if (memory_was_allocated_by_vmalloc)
> vfree(ptr);
>   else
> kfree(ptr);
> 
> but kvfree() can handle both kmalloc()ed memory and vmalloc()ed memory
> using is_vmalloc_addr(). Unless callers have special reasons, we can
> replace this branch with kvfree(). Please check and reply if you found
> problems.
> 
> Signed-off-by: Tetsuo Handa 
> Acked-by: Michal Hocko 

You can add

Acked-by: Jan Kara 

for the UDF and fs/xattr.c parts.

Honza

> Cc: Russell King  # arm
> Cc:  # apei
> Cc:  # drbd
> Cc:  # mspec
> Cc:  # drm
> Cc: Oleg Drokin  # lustre
> Cc: Andreas Dilger  # lustre
> Cc:  # coda
> Cc:  # jffs2
> Cc: Jan Kara  # udf
> Cc:  # xattr
> Cc:  # ipc + mm
> Cc:  # ipv4
> ---
>  arch/arm/mm/dma-mapping.c  | 11 ++--
>  drivers/acpi/apei/erst.c   |  6 ++--
>  drivers/block/drbd/drbd_bitmap.c   | 26 +
>  drivers/block/drbd/drbd_int.h  |  3 --
>  drivers/char/mspec.c   | 15 ++
>  drivers/gpu/drm/drm_hashtab.c  |  5 +---
>  .../lustre/include/linux/libcfs/libcfs_private.h   |  8 ++
>  fs/coda/coda_linux.h   |  3 +-
>  fs/jffs2/build.c   |  8 ++
>  fs/jffs2/fs.c  |  5 +---
>  fs/jffs2/super.c   |  5 +---
>  fs/udf/super.c |  7 +
>  fs/xattr.c | 33 
> ++
>  ipc/sem.c  |  2 +-
>  ipc/util.c | 11 ++--
>  ipc/util.h |  2 +-
>  mm/percpu.c| 18 +---
>  mm/vmalloc.c   |  5 +---
>  net/ipv4/fib_trie.c|  4 +--
>  19 files changed, 46 insertions(+), 131 deletions(-)
> 
> diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c
> index e62400e..492bf3e 100644
> --- a/arch/arm/mm/dma-mapping.c
> +++ b/arch/arm/mm/dma-mapping.c
> @@ -1200,10 +1200,7 @@ error:
>   while (i--)
>   if (pages[i])
>   __free_pages(pages[i], 0);
> - if (array_size <= PAGE_SIZE)
> - kfree(pages);
> - else
> - vfree(pages);
> + kvfree(pages);
>   return NULL;
>  }
>  
> @@ -1211,7 +1208,6 @@ static int __iommu_free_buffer(struct device *dev, 
> struct page **pages,
>  size_t size, struct dma_attrs *attrs)
>  {
>   int count = size >> PAGE_SHIFT;
> - int array_size = count * sizeof(struct page *);
>   int i;
>  
>   if (dma_get_attr(DMA_ATTR_FORCE_CONTIGUOUS, attrs)) {
> @@ -1222,10 +1218,7 @@ static int __iommu_free_buffer(struct device *dev, 
> struct page **pages,
>   __free_pages(pages[i], 0);
>   }
>  
> - if (array_size <= PAGE_SIZE)
> - kfree(pages);
> - else
> - vfree(pages);
> + kvfree(pages);
>   return 0;
>  }
>  
> diff --git a/drivers/acpi/apei/erst.c b/drivers/acpi/apei/erst.c
> index 6682c5d..6e6bc10 100644
> --- a/drivers/acpi/apei/erst.c
> +++ b/drivers/acpi/apei/erst.c
> @@ -32,6 +32,7 @@
>  #include 
>  #include 
>  #include 
> +#include  /* kvfree() */
>  #include 
>  
>  #include "apei-internal.h"
> @@ -532,10 +533,7 @@ retry:
>   return -ENOMEM;
>   memcpy(new_entries, entries,
>  erst_record_id_cache.len * sizeof(entries[0]));
> - if (erst_record_id_cache.size < PAGE_SIZE)
> - kfree(entries);
> - else
> - vfree(entries);
> + kvfree(entries);
>   erst_record_id_cache.entries = entries = new_entries;
>   erst_record_id_cache.size = new_size;
>   }
> diff --git a/drivers/block/drbd/drbd_bitmap.c 
> b/drivers/block/drbd/drbd_bitmap.c
> index 9462d27..2daaafb 100644
> --- a/drivers/block/drbd/drbd_bitmap.c
> +++ b/drivers/block/drbd/drbd_bitmap.c
> @@ -364,12 +364,9 @@ static void bm_free_pages(struct page **pages, unsigned 
> long number)
>   }
>  }
>  
> -static void bm_vk_free(void *ptr, int v)
> +static inline void bm_vk_free(void *ptr)
>  {
> - if (v)
> - vfree(ptr);
> -

Re: [PATCH] tree wide: Use kvfree() than conditional kfree()/vfree()

[Andy Shevchenko]

On Sun, Nov 8, 2015 at 7:04 AM, Tetsuo Handa
 wrote:
> Andy Shevchenko wrote:
>> Like Joe noticed you have left few places like
>> void my_func_kvfree(arg)
>> {
>> kvfree(arg);
>> }
>>
>> Might make sense to remove them completely, especially in case when
>> you have changed the callers.
>
> I think we should stop at
>
> #define my_func_kvfree(arg) kvfree(arg)

I don't think it's a good idea.

>
> in case someone want to add some code in future.

…then leave them to decide what to do, no? Trying to hunt the problem
which rather will not happen.

>
> Also, we might want to add a helper that does vmalloc() when
> kmalloc() failed because locations that do
>
>   ptr = kmalloc(size, GFP_NOFS);
>   if (!ptr)
>   ptr = vmalloc(size); /* Wrong because GFP_KERNEL is used implicitly */
>
> are found.

Another patch like Sergey suggested.

>
>> One more thought. Might be good to provide a coccinelle script for
>> such places? Julia?
>
> Welcome. I'm sure I'm missing some locations.



-- 
With Best Regards,
Andy Shevchenko
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@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] tree wide: Use kvfree() than conditional kfree()/vfree()

[Andy Shevchenko]

On Sun, Nov 8, 2015 at 7:04 AM, Tetsuo Handa
 wrote:
> Andy Shevchenko wrote:
>> Like Joe noticed you have left few places like
>> void my_func_kvfree(arg)
>> {
>> kvfree(arg);
>> }
>>
>> Might make sense to remove them completely, especially in case when
>> you have changed the callers.
>
> I think we should stop at
>
> #define my_func_kvfree(arg) kvfree(arg)

I don't think it's a good idea.

>
> in case someone want to add some code in future.

…then leave them to decide what to do, no? Trying to hunt the problem
which rather will not happen.

>
> Also, we might want to add a helper that does vmalloc() when
> kmalloc() failed because locations that do
>
>   ptr = kmalloc(size, GFP_NOFS);
>   if (!ptr)
>   ptr = vmalloc(size); /* Wrong because GFP_KERNEL is used implicitly */
>
> are found.

Another patch like Sergey suggested.

>
>> One more thought. Might be good to provide a coccinelle script for
>> such places? Julia?
>
> Welcome. I'm sure I'm missing some locations.



-- 
With Best Regards,
Andy Shevchenko
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@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] tree wide: Use kvfree() than conditional kfree()/vfree()

[Sergey Senozhatsky]

On (11/08/15 14:04), Tetsuo Handa wrote:
[..]
> Also, we might want to add a helper that does vmalloc() when
> kmalloc() failed because locations that do
> 
>   ptr = kmalloc(size, GFP_NOFS);
>   if (!ptr)
>   ptr = vmalloc(size); /* Wrong because GFP_KERNEL is used implicitly */
> 
> are found.


ext4 does something like that.


void *ext4_kvmalloc(size_t size, gfp_t flags)
{
void *ret;

ret = kmalloc(size, flags | __GFP_NOWARN);
if (!ret)
ret = __vmalloc(size, flags, PAGE_KERNEL);
return ret;
}

-ss
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@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] tree wide: Use kvfree() than conditional kfree()/vfree()

[Tetsuo Handa]

Andy Shevchenko wrote:
> Like Joe noticed you have left few places like
> void my_func_kvfree(arg)
> {
> kvfree(arg);
> }
>
> Might make sense to remove them completely, especially in case when
> you have changed the callers.

I think we should stop at

#define my_func_kvfree(arg) kvfree(arg)

in case someone want to add some code in future.

Also, we might want to add a helper that does vmalloc() when
kmalloc() failed because locations that do

  ptr = kmalloc(size, GFP_NOFS);
  if (!ptr)
  ptr = vmalloc(size); /* Wrong because GFP_KERNEL is used implicitly */

are found.

> One more thought. Might be good to provide a coccinelle script for
> such places? Julia?

Welcome. I'm sure I'm missing some locations.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@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] tree wide: Use kvfree() than conditional kfree()/vfree()

[Andy Shevchenko]

On Sat, Nov 7, 2015 at 10:05 PM, Andy Shevchenko
 wrote:
> On Sat, Nov 7, 2015 at 1:44 PM, Tetsuo Handa
>  wrote:
>> There are many locations that do
>>
>>   if (memory_was_allocated_by_vmalloc)
>> vfree(ptr);
>>   else
>> kfree(ptr);
>>
>> but kvfree() can handle both kmalloc()ed memory and vmalloc()ed memory
>> using is_vmalloc_addr(). Unless callers have special reasons, we can
>> replace this branch with kvfree().
>>
>
> Like Joe noticed you have left few places like
> void my_func_kvfree(arg)
> {
> kvfree(arg);
> }
>
> Might make sense to remove them completely, especially in case when
> you have changed the callers.
>

One more thought. Might be good to provide a coccinelle script for
such places? Julia?

>> Signed-off-by: Tetsuo Handa 
>> ---
>>  arch/arm/mm/dma-mapping.c  | 11 ++--
>>  drivers/acpi/apei/erst.c   |  6 ++--
>>  drivers/block/drbd/drbd_bitmap.c   | 26 +
>>  drivers/block/drbd/drbd_int.h  |  3 --
>>  drivers/char/mspec.c   | 15 ++
>>  drivers/gpu/drm/drm_hashtab.c  |  5 +---
>>  .../lustre/include/linux/libcfs/libcfs_private.h   |  8 ++
>>  fs/coda/coda_linux.h   |  3 +-
>>  fs/jffs2/build.c   |  8 ++
>>  fs/jffs2/fs.c  |  5 +---
>>  fs/jffs2/super.c   |  5 +---
>>  fs/udf/super.c |  7 +
>>  fs/xattr.c | 33 
>> ++
>>  ipc/sem.c  |  2 +-
>>  ipc/util.c |  8 ++
>>  ipc/util.h |  2 +-
>>  mm/percpu.c| 18 +---
>>  mm/vmalloc.c   |  5 +---
>>  net/ipv4/fib_trie.c|  4 +--
>>  19 files changed, 45 insertions(+), 129 deletions(-)
>>
>> diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c
>> index e62400e..492bf3e 100644
>> --- a/arch/arm/mm/dma-mapping.c
>> +++ b/arch/arm/mm/dma-mapping.c
>> @@ -1200,10 +1200,7 @@ error:
>> while (i--)
>> if (pages[i])
>> __free_pages(pages[i], 0);
>> -   if (array_size <= PAGE_SIZE)
>> -   kfree(pages);
>> -   else
>> -   vfree(pages);
>> +   kvfree(pages);
>> return NULL;
>>  }
>>
>> @@ -1211,7 +1208,6 @@ static int __iommu_free_buffer(struct device *dev, 
>> struct page **pages,
>>size_t size, struct dma_attrs *attrs)
>>  {
>> int count = size >> PAGE_SHIFT;
>> -   int array_size = count * sizeof(struct page *);
>> int i;
>>
>> if (dma_get_attr(DMA_ATTR_FORCE_CONTIGUOUS, attrs)) {
>> @@ -1222,10 +1218,7 @@ static int __iommu_free_buffer(struct device *dev, 
>> struct page **pages,
>> __free_pages(pages[i], 0);
>> }
>>
>> -   if (array_size <= PAGE_SIZE)
>> -   kfree(pages);
>> -   else
>> -   vfree(pages);
>> +   kvfree(pages);
>> return 0;
>>  }
>>
>> diff --git a/drivers/acpi/apei/erst.c b/drivers/acpi/apei/erst.c
>> index 6682c5d..6e6bc10 100644
>> --- a/drivers/acpi/apei/erst.c
>> +++ b/drivers/acpi/apei/erst.c
>> @@ -32,6 +32,7 @@
>>  #include 
>>  #include 
>>  #include 
>> +#include  /* kvfree() */
>>  #include 
>>
>>  #include "apei-internal.h"
>> @@ -532,10 +533,7 @@ retry:
>> return -ENOMEM;
>> memcpy(new_entries, entries,
>>erst_record_id_cache.len * sizeof(entries[0]));
>> -   if (erst_record_id_cache.size < PAGE_SIZE)
>> -   kfree(entries);
>> -   else
>> -   vfree(entries);
>> +   kvfree(entries);
>> erst_record_id_cache.entries = entries = new_entries;
>> erst_record_id_cache.size = new_size;
>> }
>> diff --git a/drivers/block/drbd/drbd_bitmap.c 
>> b/drivers/block/drbd/drbd_bitmap.c
>> index 9462d27..a090fb7 100644
>> --- a/drivers/block/drbd/drbd_bitmap.c
>> +++ b/drivers/block/drbd/drbd_bitmap.c
>> @@ -364,12 +364,9 @@ static void bm_free_pages(struct page **pages, unsigned 
>> long number)
>> }
>>  }
>>
>> -static void bm_vk_free(void *ptr, int v)
>> +static void bm_vk_free(void *ptr)
>>  {
>> -   if (v)
>> -   vfree(ptr);
>> -   else
>> -   kfree(ptr);
>> +   kvfree(ptr);
>>  }
>>
>>  /*
>> @@ -379,7 +376,7 @@ static struct page **bm_realloc_pages(struct drbd_bitmap 
>> *b, unsigned long want)
>>  {
>> struct page **old_pages = b->bm_pages;
>> struct page **new_pages, *page;
>> -   unsigned int i, bytes, vmalloced = 0;

Re: [PATCH] tree wide: Use kvfree() than conditional kfree()/vfree()

[Andy Shevchenko]

On Sat, Nov 7, 2015 at 1:44 PM, Tetsuo Handa
 wrote:
> There are many locations that do
>
>   if (memory_was_allocated_by_vmalloc)
> vfree(ptr);
>   else
> kfree(ptr);
>
> but kvfree() can handle both kmalloc()ed memory and vmalloc()ed memory
> using is_vmalloc_addr(). Unless callers have special reasons, we can
> replace this branch with kvfree().
>

Like Joe noticed you have left few places like
void my_func_kvfree(arg)
{
kvfree(arg);
}

Might make sense to remove them completely, especially in case when
you have changed the callers.

> Signed-off-by: Tetsuo Handa 
> ---
>  arch/arm/mm/dma-mapping.c  | 11 ++--
>  drivers/acpi/apei/erst.c   |  6 ++--
>  drivers/block/drbd/drbd_bitmap.c   | 26 +
>  drivers/block/drbd/drbd_int.h  |  3 --
>  drivers/char/mspec.c   | 15 ++
>  drivers/gpu/drm/drm_hashtab.c  |  5 +---
>  .../lustre/include/linux/libcfs/libcfs_private.h   |  8 ++
>  fs/coda/coda_linux.h   |  3 +-
>  fs/jffs2/build.c   |  8 ++
>  fs/jffs2/fs.c  |  5 +---
>  fs/jffs2/super.c   |  5 +---
>  fs/udf/super.c |  7 +
>  fs/xattr.c | 33 
> ++
>  ipc/sem.c  |  2 +-
>  ipc/util.c |  8 ++
>  ipc/util.h |  2 +-
>  mm/percpu.c| 18 +---
>  mm/vmalloc.c   |  5 +---
>  net/ipv4/fib_trie.c|  4 +--
>  19 files changed, 45 insertions(+), 129 deletions(-)
>
> diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c
> index e62400e..492bf3e 100644
> --- a/arch/arm/mm/dma-mapping.c
> +++ b/arch/arm/mm/dma-mapping.c
> @@ -1200,10 +1200,7 @@ error:
> while (i--)
> if (pages[i])
> __free_pages(pages[i], 0);
> -   if (array_size <= PAGE_SIZE)
> -   kfree(pages);
> -   else
> -   vfree(pages);
> +   kvfree(pages);
> return NULL;
>  }
>
> @@ -1211,7 +1208,6 @@ static int __iommu_free_buffer(struct device *dev, 
> struct page **pages,
>size_t size, struct dma_attrs *attrs)
>  {
> int count = size >> PAGE_SHIFT;
> -   int array_size = count * sizeof(struct page *);
> int i;
>
> if (dma_get_attr(DMA_ATTR_FORCE_CONTIGUOUS, attrs)) {
> @@ -1222,10 +1218,7 @@ static int __iommu_free_buffer(struct device *dev, 
> struct page **pages,
> __free_pages(pages[i], 0);
> }
>
> -   if (array_size <= PAGE_SIZE)
> -   kfree(pages);
> -   else
> -   vfree(pages);
> +   kvfree(pages);
> return 0;
>  }
>
> diff --git a/drivers/acpi/apei/erst.c b/drivers/acpi/apei/erst.c
> index 6682c5d..6e6bc10 100644
> --- a/drivers/acpi/apei/erst.c
> +++ b/drivers/acpi/apei/erst.c
> @@ -32,6 +32,7 @@
>  #include 
>  #include 
>  #include 
> +#include  /* kvfree() */
>  #include 
>
>  #include "apei-internal.h"
> @@ -532,10 +533,7 @@ retry:
> return -ENOMEM;
> memcpy(new_entries, entries,
>erst_record_id_cache.len * sizeof(entries[0]));
> -   if (erst_record_id_cache.size < PAGE_SIZE)
> -   kfree(entries);
> -   else
> -   vfree(entries);
> +   kvfree(entries);
> erst_record_id_cache.entries = entries = new_entries;
> erst_record_id_cache.size = new_size;
> }
> diff --git a/drivers/block/drbd/drbd_bitmap.c 
> b/drivers/block/drbd/drbd_bitmap.c
> index 9462d27..a090fb7 100644
> --- a/drivers/block/drbd/drbd_bitmap.c
> +++ b/drivers/block/drbd/drbd_bitmap.c
> @@ -364,12 +364,9 @@ static void bm_free_pages(struct page **pages, unsigned 
> long number)
> }
>  }
>
> -static void bm_vk_free(void *ptr, int v)
> +static void bm_vk_free(void *ptr)
>  {
> -   if (v)
> -   vfree(ptr);
> -   else
> -   kfree(ptr);
> +   kvfree(ptr);
>  }
>
>  /*
> @@ -379,7 +376,7 @@ static struct page **bm_realloc_pages(struct drbd_bitmap 
> *b, unsigned long want)
>  {
> struct page **old_pages = b->bm_pages;
> struct page **new_pages, *page;
> -   unsigned int i, bytes, vmalloced = 0;
> +   unsigned int i, bytes;
> unsigned long have = b->bm_number_of_pages;
>
> BUG_ON(have == 0 && old_pages != NULL);
> @@ -401,7 +398,6 @@ static struct page **bm_realloc_pages(struct drbd_bitmap 
> *b, unsigned long want)
> PAGE_KERNEL);

Re: [PATCH] tree wide: Use kvfree() than conditional kfree()/vfree()

[Joe Perches]

On Sat, 2015-11-07 at 20:44 +0900, Tetsuo Handa wrote:
> bm_vk_free

Might as well get rid of the static function altogether.
Maybe in a follow-on patch.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH] tree wide: Use kvfree() than conditional kfree()/vfree()

[Tetsuo Handa]

There are many locations that do

  if (memory_was_allocated_by_vmalloc)
vfree(ptr);
  else
kfree(ptr);

but kvfree() can handle both kmalloc()ed memory and vmalloc()ed memory
using is_vmalloc_addr(). Unless callers have special reasons, we can
replace this branch with kvfree().

Signed-off-by: Tetsuo Handa 
---
 arch/arm/mm/dma-mapping.c  | 11 ++--
 drivers/acpi/apei/erst.c   |  6 ++--
 drivers/block/drbd/drbd_bitmap.c   | 26 +
 drivers/block/drbd/drbd_int.h  |  3 --
 drivers/char/mspec.c   | 15 ++
 drivers/gpu/drm/drm_hashtab.c  |  5 +---
 .../lustre/include/linux/libcfs/libcfs_private.h   |  8 ++
 fs/coda/coda_linux.h   |  3 +-
 fs/jffs2/build.c   |  8 ++
 fs/jffs2/fs.c  |  5 +---
 fs/jffs2/super.c   |  5 +---
 fs/udf/super.c |  7 +
 fs/xattr.c | 33 ++
 ipc/sem.c  |  2 +-
 ipc/util.c |  8 ++
 ipc/util.h |  2 +-
 mm/percpu.c| 18 +---
 mm/vmalloc.c   |  5 +---
 net/ipv4/fib_trie.c|  4 +--
 19 files changed, 45 insertions(+), 129 deletions(-)

diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c
index e62400e..492bf3e 100644
--- a/arch/arm/mm/dma-mapping.c
+++ b/arch/arm/mm/dma-mapping.c
@@ -1200,10 +1200,7 @@ error:
while (i--)
if (pages[i])
__free_pages(pages[i], 0);
-   if (array_size <= PAGE_SIZE)
-   kfree(pages);
-   else
-   vfree(pages);
+   kvfree(pages);
return NULL;
 }
 
@@ -1211,7 +1208,6 @@ static int __iommu_free_buffer(struct device *dev, struct 
page **pages,
   size_t size, struct dma_attrs *attrs)
 {
int count = size >> PAGE_SHIFT;
-   int array_size = count * sizeof(struct page *);
int i;
 
if (dma_get_attr(DMA_ATTR_FORCE_CONTIGUOUS, attrs)) {
@@ -1222,10 +1218,7 @@ static int __iommu_free_buffer(struct device *dev, 
struct page **pages,
__free_pages(pages[i], 0);
}
 
-   if (array_size <= PAGE_SIZE)
-   kfree(pages);
-   else
-   vfree(pages);
+   kvfree(pages);
return 0;
 }
 
diff --git a/drivers/acpi/apei/erst.c b/drivers/acpi/apei/erst.c
index 6682c5d..6e6bc10 100644
--- a/drivers/acpi/apei/erst.c
+++ b/drivers/acpi/apei/erst.c
@@ -32,6 +32,7 @@
 #include 
 #include 
 #include 
+#include  /* kvfree() */
 #include 
 
 #include "apei-internal.h"
@@ -532,10 +533,7 @@ retry:
return -ENOMEM;
memcpy(new_entries, entries,
   erst_record_id_cache.len * sizeof(entries[0]));
-   if (erst_record_id_cache.size < PAGE_SIZE)
-   kfree(entries);
-   else
-   vfree(entries);
+   kvfree(entries);
erst_record_id_cache.entries = entries = new_entries;
erst_record_id_cache.size = new_size;
}
diff --git a/drivers/block/drbd/drbd_bitmap.c b/drivers/block/drbd/drbd_bitmap.c
index 9462d27..a090fb7 100644
--- a/drivers/block/drbd/drbd_bitmap.c
+++ b/drivers/block/drbd/drbd_bitmap.c
@@ -364,12 +364,9 @@ static void bm_free_pages(struct page **pages, unsigned 
long number)
}
 }
 
-static void bm_vk_free(void *ptr, int v)
+static void bm_vk_free(void *ptr)
 {
-   if (v)
-   vfree(ptr);
-   else
-   kfree(ptr);
+   kvfree(ptr);
 }
 
 /*
@@ -379,7 +376,7 @@ static struct page **bm_realloc_pages(struct drbd_bitmap 
*b, unsigned long want)
 {
struct page **old_pages = b->bm_pages;
struct page **new_pages, *page;
-   unsigned int i, bytes, vmalloced = 0;
+   unsigned int i, bytes;
unsigned long have = b->bm_number_of_pages;
 
BUG_ON(have == 0 && old_pages != NULL);
@@ -401,7 +398,6 @@ static struct page **bm_realloc_pages(struct drbd_bitmap 
*b, unsigned long want)
PAGE_KERNEL);
if (!new_pages)
return NULL;
-   vmalloced = 1;
}
 
if (want >= have) {
@@ -411,7 +407,7 @@ static struct page **bm_realloc_pages(struct drbd_bitmap 
*b, unsigned long want)
page = alloc_page(GFP_NOIO | __GFP_HIGHMEM);
if (!page) {
bm_free_pages(new_pages + have, i - have);
-   bm_vk_free(new_pages, 

[PATCH] tree wide: Use kvfree() than conditional kfree()/vfree()

[Tetsuo Handa]

There are many locations that do

  if (memory_was_allocated_by_vmalloc)
vfree(ptr);
  else
kfree(ptr);

but kvfree() can handle both kmalloc()ed memory and vmalloc()ed memory
using is_vmalloc_addr(). Unless callers have special reasons, we can
replace this branch with kvfree().

Signed-off-by: Tetsuo Handa 
---
 arch/arm/mm/dma-mapping.c  | 11 ++--
 drivers/acpi/apei/erst.c   |  6 ++--
 drivers/block/drbd/drbd_bitmap.c   | 26 +
 drivers/block/drbd/drbd_int.h  |  3 --
 drivers/char/mspec.c   | 15 ++
 drivers/gpu/drm/drm_hashtab.c  |  5 +---
 .../lustre/include/linux/libcfs/libcfs_private.h   |  8 ++
 fs/coda/coda_linux.h   |  3 +-
 fs/jffs2/build.c   |  8 ++
 fs/jffs2/fs.c  |  5 +---
 fs/jffs2/super.c   |  5 +---
 fs/udf/super.c |  7 +
 fs/xattr.c | 33 ++
 ipc/sem.c  |  2 +-
 ipc/util.c |  8 ++
 ipc/util.h |  2 +-
 mm/percpu.c| 18 +---
 mm/vmalloc.c   |  5 +---
 net/ipv4/fib_trie.c|  4 +--
 19 files changed, 45 insertions(+), 129 deletions(-)

diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c
index e62400e..492bf3e 100644
--- a/arch/arm/mm/dma-mapping.c
+++ b/arch/arm/mm/dma-mapping.c
@@ -1200,10 +1200,7 @@ error:
while (i--)
if (pages[i])
__free_pages(pages[i], 0);
-   if (array_size <= PAGE_SIZE)
-   kfree(pages);
-   else
-   vfree(pages);
+   kvfree(pages);
return NULL;
 }
 
@@ -1211,7 +1208,6 @@ static int __iommu_free_buffer(struct device *dev, struct 
page **pages,
   size_t size, struct dma_attrs *attrs)
 {
int count = size >> PAGE_SHIFT;
-   int array_size = count * sizeof(struct page *);
int i;
 
if (dma_get_attr(DMA_ATTR_FORCE_CONTIGUOUS, attrs)) {
@@ -1222,10 +1218,7 @@ static int __iommu_free_buffer(struct device *dev, 
struct page **pages,
__free_pages(pages[i], 0);
}
 
-   if (array_size <= PAGE_SIZE)
-   kfree(pages);
-   else
-   vfree(pages);
+   kvfree(pages);
return 0;
 }
 
diff --git a/drivers/acpi/apei/erst.c b/drivers/acpi/apei/erst.c
index 6682c5d..6e6bc10 100644
--- a/drivers/acpi/apei/erst.c
+++ b/drivers/acpi/apei/erst.c
@@ -32,6 +32,7 @@
 #include 
 #include 
 #include 
+#include  /* kvfree() */
 #include 
 
 #include "apei-internal.h"
@@ -532,10 +533,7 @@ retry:
return -ENOMEM;
memcpy(new_entries, entries,
   erst_record_id_cache.len * sizeof(entries[0]));
-   if (erst_record_id_cache.size < PAGE_SIZE)
-   kfree(entries);
-   else
-   vfree(entries);
+   kvfree(entries);
erst_record_id_cache.entries = entries = new_entries;
erst_record_id_cache.size = new_size;
}
diff --git a/drivers/block/drbd/drbd_bitmap.c b/drivers/block/drbd/drbd_bitmap.c
index 9462d27..a090fb7 100644
--- a/drivers/block/drbd/drbd_bitmap.c
+++ b/drivers/block/drbd/drbd_bitmap.c
@@ -364,12 +364,9 @@ static void bm_free_pages(struct page **pages, unsigned 
long number)
}
 }
 
-static void bm_vk_free(void *ptr, int v)
+static void bm_vk_free(void *ptr)
 {
-   if (v)
-   vfree(ptr);
-   else
-   kfree(ptr);
+   kvfree(ptr);
 }
 
 /*
@@ -379,7 +376,7 @@ static struct page **bm_realloc_pages(struct drbd_bitmap 
*b, unsigned long want)
 {
struct page **old_pages = b->bm_pages;
struct page **new_pages, *page;
-   unsigned int i, bytes, vmalloced = 0;
+   unsigned int i, bytes;
unsigned long have = b->bm_number_of_pages;
 
BUG_ON(have == 0 && old_pages != NULL);
@@ -401,7 +398,6 @@ static struct page **bm_realloc_pages(struct drbd_bitmap 
*b, unsigned long want)
PAGE_KERNEL);
if (!new_pages)
return NULL;
-   vmalloced = 1;
}
 
if (want >= have) {
@@ -411,7 +407,7 @@ static struct page **bm_realloc_pages(struct drbd_bitmap 
*b, unsigned long want)
page = alloc_page(GFP_NOIO | __GFP_HIGHMEM);
if (!page) {
bm_free_pages(new_pages + have, i - have);
-   

Re: [PATCH] tree wide: Use kvfree() than conditional kfree()/vfree()

[Joe Perches]

On Sat, 2015-11-07 at 20:44 +0900, Tetsuo Handa wrote:
> bm_vk_free

Might as well get rid of the static function altogether.
Maybe in a follow-on patch.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@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] tree wide: Use kvfree() than conditional kfree()/vfree()

[Andy Shevchenko]

On Sat, Nov 7, 2015 at 1:44 PM, Tetsuo Handa
 wrote:
> There are many locations that do
>
>   if (memory_was_allocated_by_vmalloc)
> vfree(ptr);
>   else
> kfree(ptr);
>
> but kvfree() can handle both kmalloc()ed memory and vmalloc()ed memory
> using is_vmalloc_addr(). Unless callers have special reasons, we can
> replace this branch with kvfree().
>

Like Joe noticed you have left few places like
void my_func_kvfree(arg)
{
kvfree(arg);
}

Might make sense to remove them completely, especially in case when
you have changed the callers.

> Signed-off-by: Tetsuo Handa 
> ---
>  arch/arm/mm/dma-mapping.c  | 11 ++--
>  drivers/acpi/apei/erst.c   |  6 ++--
>  drivers/block/drbd/drbd_bitmap.c   | 26 +
>  drivers/block/drbd/drbd_int.h  |  3 --
>  drivers/char/mspec.c   | 15 ++
>  drivers/gpu/drm/drm_hashtab.c  |  5 +---
>  .../lustre/include/linux/libcfs/libcfs_private.h   |  8 ++
>  fs/coda/coda_linux.h   |  3 +-
>  fs/jffs2/build.c   |  8 ++
>  fs/jffs2/fs.c  |  5 +---
>  fs/jffs2/super.c   |  5 +---
>  fs/udf/super.c |  7 +
>  fs/xattr.c | 33 
> ++
>  ipc/sem.c  |  2 +-
>  ipc/util.c |  8 ++
>  ipc/util.h |  2 +-
>  mm/percpu.c| 18 +---
>  mm/vmalloc.c   |  5 +---
>  net/ipv4/fib_trie.c|  4 +--
>  19 files changed, 45 insertions(+), 129 deletions(-)
>
> diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c
> index e62400e..492bf3e 100644
> --- a/arch/arm/mm/dma-mapping.c
> +++ b/arch/arm/mm/dma-mapping.c
> @@ -1200,10 +1200,7 @@ error:
> while (i--)
> if (pages[i])
> __free_pages(pages[i], 0);
> -   if (array_size <= PAGE_SIZE)
> -   kfree(pages);
> -   else
> -   vfree(pages);
> +   kvfree(pages);
> return NULL;
>  }
>
> @@ -1211,7 +1208,6 @@ static int __iommu_free_buffer(struct device *dev, 
> struct page **pages,
>size_t size, struct dma_attrs *attrs)
>  {
> int count = size >> PAGE_SHIFT;
> -   int array_size = count * sizeof(struct page *);
> int i;
>
> if (dma_get_attr(DMA_ATTR_FORCE_CONTIGUOUS, attrs)) {
> @@ -1222,10 +1218,7 @@ static int __iommu_free_buffer(struct device *dev, 
> struct page **pages,
> __free_pages(pages[i], 0);
> }
>
> -   if (array_size <= PAGE_SIZE)
> -   kfree(pages);
> -   else
> -   vfree(pages);
> +   kvfree(pages);
> return 0;
>  }
>
> diff --git a/drivers/acpi/apei/erst.c b/drivers/acpi/apei/erst.c
> index 6682c5d..6e6bc10 100644
> --- a/drivers/acpi/apei/erst.c
> +++ b/drivers/acpi/apei/erst.c
> @@ -32,6 +32,7 @@
>  #include 
>  #include 
>  #include 
> +#include  /* kvfree() */
>  #include 
>
>  #include "apei-internal.h"
> @@ -532,10 +533,7 @@ retry:
> return -ENOMEM;
> memcpy(new_entries, entries,
>erst_record_id_cache.len * sizeof(entries[0]));
> -   if (erst_record_id_cache.size < PAGE_SIZE)
> -   kfree(entries);
> -   else
> -   vfree(entries);
> +   kvfree(entries);
> erst_record_id_cache.entries = entries = new_entries;
> erst_record_id_cache.size = new_size;
> }
> diff --git a/drivers/block/drbd/drbd_bitmap.c 
> b/drivers/block/drbd/drbd_bitmap.c
> index 9462d27..a090fb7 100644
> --- a/drivers/block/drbd/drbd_bitmap.c
> +++ b/drivers/block/drbd/drbd_bitmap.c
> @@ -364,12 +364,9 @@ static void bm_free_pages(struct page **pages, unsigned 
> long number)
> }
>  }
>
> -static void bm_vk_free(void *ptr, int v)
> +static void bm_vk_free(void *ptr)
>  {
> -   if (v)
> -   vfree(ptr);
> -   else
> -   kfree(ptr);
> +   kvfree(ptr);
>  }
>
>  /*
> @@ -379,7 +376,7 @@ static struct page **bm_realloc_pages(struct drbd_bitmap 
> *b, unsigned long want)
>  {
> struct page **old_pages = b->bm_pages;
> struct page **new_pages, *page;
> -   unsigned int i, bytes, vmalloced = 0;
> +   unsigned int i, bytes;
> unsigned long have = b->bm_number_of_pages;
>
> BUG_ON(have == 0 && old_pages != NULL);
> @@ -401,7 +398,6 @@ static struct page **bm_realloc_pages(struct drbd_bitmap 
> 

Re: [PATCH] tree wide: Use kvfree() than conditional kfree()/vfree()

[Andy Shevchenko]

On Sat, Nov 7, 2015 at 10:05 PM, Andy Shevchenko
 wrote:
> On Sat, Nov 7, 2015 at 1:44 PM, Tetsuo Handa
>  wrote:
>> There are many locations that do
>>
>>   if (memory_was_allocated_by_vmalloc)
>> vfree(ptr);
>>   else
>> kfree(ptr);
>>
>> but kvfree() can handle both kmalloc()ed memory and vmalloc()ed memory
>> using is_vmalloc_addr(). Unless callers have special reasons, we can
>> replace this branch with kvfree().
>>
>
> Like Joe noticed you have left few places like
> void my_func_kvfree(arg)
> {
> kvfree(arg);
> }
>
> Might make sense to remove them completely, especially in case when
> you have changed the callers.
>

One more thought. Might be good to provide a coccinelle script for
such places? Julia?

>> Signed-off-by: Tetsuo Handa 
>> ---
>>  arch/arm/mm/dma-mapping.c  | 11 ++--
>>  drivers/acpi/apei/erst.c   |  6 ++--
>>  drivers/block/drbd/drbd_bitmap.c   | 26 +
>>  drivers/block/drbd/drbd_int.h  |  3 --
>>  drivers/char/mspec.c   | 15 ++
>>  drivers/gpu/drm/drm_hashtab.c  |  5 +---
>>  .../lustre/include/linux/libcfs/libcfs_private.h   |  8 ++
>>  fs/coda/coda_linux.h   |  3 +-
>>  fs/jffs2/build.c   |  8 ++
>>  fs/jffs2/fs.c  |  5 +---
>>  fs/jffs2/super.c   |  5 +---
>>  fs/udf/super.c |  7 +
>>  fs/xattr.c | 33 
>> ++
>>  ipc/sem.c  |  2 +-
>>  ipc/util.c |  8 ++
>>  ipc/util.h |  2 +-
>>  mm/percpu.c| 18 +---
>>  mm/vmalloc.c   |  5 +---
>>  net/ipv4/fib_trie.c|  4 +--
>>  19 files changed, 45 insertions(+), 129 deletions(-)
>>
>> diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c
>> index e62400e..492bf3e 100644
>> --- a/arch/arm/mm/dma-mapping.c
>> +++ b/arch/arm/mm/dma-mapping.c
>> @@ -1200,10 +1200,7 @@ error:
>> while (i--)
>> if (pages[i])
>> __free_pages(pages[i], 0);
>> -   if (array_size <= PAGE_SIZE)
>> -   kfree(pages);
>> -   else
>> -   vfree(pages);
>> +   kvfree(pages);
>> return NULL;
>>  }
>>
>> @@ -1211,7 +1208,6 @@ static int __iommu_free_buffer(struct device *dev, 
>> struct page **pages,
>>size_t size, struct dma_attrs *attrs)
>>  {
>> int count = size >> PAGE_SHIFT;
>> -   int array_size = count * sizeof(struct page *);
>> int i;
>>
>> if (dma_get_attr(DMA_ATTR_FORCE_CONTIGUOUS, attrs)) {
>> @@ -1222,10 +1218,7 @@ static int __iommu_free_buffer(struct device *dev, 
>> struct page **pages,
>> __free_pages(pages[i], 0);
>> }
>>
>> -   if (array_size <= PAGE_SIZE)
>> -   kfree(pages);
>> -   else
>> -   vfree(pages);
>> +   kvfree(pages);
>> return 0;
>>  }
>>
>> diff --git a/drivers/acpi/apei/erst.c b/drivers/acpi/apei/erst.c
>> index 6682c5d..6e6bc10 100644
>> --- a/drivers/acpi/apei/erst.c
>> +++ b/drivers/acpi/apei/erst.c
>> @@ -32,6 +32,7 @@
>>  #include 
>>  #include 
>>  #include 
>> +#include  /* kvfree() */
>>  #include 
>>
>>  #include "apei-internal.h"
>> @@ -532,10 +533,7 @@ retry:
>> return -ENOMEM;
>> memcpy(new_entries, entries,
>>erst_record_id_cache.len * sizeof(entries[0]));
>> -   if (erst_record_id_cache.size < PAGE_SIZE)
>> -   kfree(entries);
>> -   else
>> -   vfree(entries);
>> +   kvfree(entries);
>> erst_record_id_cache.entries = entries = new_entries;
>> erst_record_id_cache.size = new_size;
>> }
>> diff --git a/drivers/block/drbd/drbd_bitmap.c 
>> b/drivers/block/drbd/drbd_bitmap.c
>> index 9462d27..a090fb7 100644
>> --- a/drivers/block/drbd/drbd_bitmap.c
>> +++ b/drivers/block/drbd/drbd_bitmap.c
>> @@ -364,12 +364,9 @@ static void bm_free_pages(struct page **pages, unsigned 
>> long number)
>> }
>>  }
>>
>> -static void bm_vk_free(void *ptr, int v)
>> +static void bm_vk_free(void *ptr)
>>  {
>> -   if (v)
>> -   vfree(ptr);
>> -   else
>> -   kfree(ptr);
>> +   kvfree(ptr);
>>  }
>>
>>  /*
>> @@ -379,7 +376,7 @@ static struct page **bm_realloc_pages(struct drbd_bitmap 
>> *b, unsigned long want)
>>  {
>> struct page **old_pages = 

Re: [PATCH] tree wide: Use kvfree() than conditional kfree()/vfree()

[Tetsuo Handa]

Andy Shevchenko wrote:
> Like Joe noticed you have left few places like
> void my_func_kvfree(arg)
> {
> kvfree(arg);
> }
>
> Might make sense to remove them completely, especially in case when
> you have changed the callers.

I think we should stop at

#define my_func_kvfree(arg) kvfree(arg)

in case someone want to add some code in future.

Also, we might want to add a helper that does vmalloc() when
kmalloc() failed because locations that do

  ptr = kmalloc(size, GFP_NOFS);
  if (!ptr)
  ptr = vmalloc(size); /* Wrong because GFP_KERNEL is used implicitly */

are found.

> One more thought. Might be good to provide a coccinelle script for
> such places? Julia?

Welcome. I'm sure I'm missing some locations.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@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] tree wide: Use kvfree() than conditional kfree()/vfree()

[Sergey Senozhatsky]

On (11/08/15 14:04), Tetsuo Handa wrote:
[..]
> Also, we might want to add a helper that does vmalloc() when
> kmalloc() failed because locations that do
> 
>   ptr = kmalloc(size, GFP_NOFS);
>   if (!ptr)
>   ptr = vmalloc(size); /* Wrong because GFP_KERNEL is used implicitly */
> 
> are found.


ext4 does something like that.


void *ext4_kvmalloc(size_t size, gfp_t flags)
{
void *ret;

ret = kmalloc(size, flags | __GFP_NOWARN);
if (!ret)
ret = __vmalloc(size, flags, PAGE_KERNEL);
return ret;
}

-ss
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/