On Tue, Aug 20, 2013 at 12:16:53PM +0800, Chen Gang wrote:
> For handle related extern functions, recommend to consider about
> invalid handle value, or can not be easily used by callers.
>
> In our case:
>
> if need call zbud_alloc/free() multiple times, the caller need additional value to mark current status.
> the caller can not continue call zbud_free() multiple times like kfree().
> when call zbud_map(), the caller can not know about whether succeed or not.
Didn't quite understand you here. kfree() is able to handle a NULL
pointer, but double frees are not allowed.
Proper refcounting of your allocations can protect against double frees.
>
> And NULL (or 0) should be also treated as invalid handle value, since
> the internal implementation has already assumed NULL is an invalid
> address.
I'd like to keep the handle space open to all valid values that can be
represented by an unsigned long.
The return value of zbud_alloc() will tell you if the handle is valid or
not.
Not really seeing the point of this change.
Seth
>
> e.g. "struct zbud_header *zhdr = NULL;" in function 'zbud_alloc'.
>
> At least, current interface definition still has no bugs, so the common
> patch appliers is better to keep compatible with the original interface.
>
> And related maintainers can re-construct interface without considering
> the compatibility at a suitable time point, so the interface can be get
> additional improvement.
>
> e.g. for handle's type, "void *" is more commonly used than "unsigned long".
> can find support macros or functions in "include/linux/err.h" for "void *".
> but can not find any support macros or functions for "unsigned long" in "./include" sub directory.
>
> e.g. for zbud_alloc(), can return a handle directly instead of an error code.
>
>
> Signed-off-by: Chen Gang <gang.chen@xxxxxxxxxxx>
> ---
> mm/zbud.c | 12 +++++++++---
> 1 files changed, 9 insertions(+), 3 deletions(-)
>
> diff --git a/mm/zbud.c b/mm/zbud.c
> index 9451361..b5363ea 100644
> --- a/mm/zbud.c
> +++ b/mm/zbud.c
> @@ -244,8 +244,8 @@ void zbud_destroy_pool(struct zbud_pool *pool)
> * as zbud pool pages.
> *
> * Return: 0 if success and handle is set, otherwise -EINVAL if the size or
> - * gfp arguments are invalid or -ENOMEM if the pool was unable to allocate
> - * a new page.
> + * gfp arguments are invalid, or -ENOMEM if the pool was unable to allocate
> + * a new page, or -NOSPC if no space left, also set invalid value to handle.
> */
> int zbud_alloc(struct zbud_pool *pool, int size, gfp_t gfp,
> unsigned long *handle)
> @@ -255,6 +255,8 @@ int zbud_alloc(struct zbud_pool *pool, int size, gfp_t gfp,
> enum buddy bud;
> struct page *page;
>
> + *handle = 0;
> +
> if (size <= 0 || gfp & __GFP_HIGHMEM)
> return -EINVAL;
> if (size > PAGE_SIZE - ZHDR_SIZE_ALIGNED - CHUNK_SIZE)
> @@ -328,6 +330,9 @@ void zbud_free(struct zbud_pool *pool, unsigned long handle)
> struct zbud_header *zhdr;
> int freechunks;
>
> + if (IS_ERR_OR_NULL((void *)handle))
> + return;
> +
> spin_lock(&pool->lock);
> zhdr = handle_to_zbud_header(handle);
>
> @@ -478,7 +483,8 @@ next:
> * in the handle and could create temporary mappings to make the data
> * accessible to the user.
> *
> - * Returns: a pointer to the mapped allocation
> + * Returns: a pointer to the mapped allocation, or an invalid value which can
> + * be checked by IS_ERR_OR_NULL().
> */
> void *zbud_map(struct zbud_pool *pool, unsigned long handle)
> {
> --
> 1.7.7.6
>
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@xxxxxxxxx. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>