On 08/20/2013 11:29 PM, Seth Jennings wrote:
> 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.
>
It seems, we don't allow the caller double frees, either.
Is it correct ?
> Proper refcounting of your allocations can protect against double frees.
>
So the caller need additional value to mark current status (e.g refcounting)
And can the caller know about whether succeed or not after call zbud_map() ?
>>
>> 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.
>
Also include NULL ?
> The return value of zbud_alloc() will tell you if the handle is valid or
> not.
>
Yes, but it let the caller can not use this interface, easily.
> Not really seeing the point of this change.
>
So need additional discussion. :-)
Thanks.
> 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>
>
--
Chen Gang
--
Chen Gang
--
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>