Re: [PATCH] lib/idr.c: remove duplicated bound checking in sub_alloc

Re: [PATCH] lib/idr.c: remove duplicated bound checking in sub_alloc

[Andrew Morton]

On Thu, 19 Mar 2015 17:57:07 +0000 Liu Sha <rednoax@qq.com> wrote:

> From: Liu Sha <rednoax@gmail.com>
> 
> The INT_MAX bound checking in sub_alloc checks two conditions to see
> whether the signed integer "id" is beyond INT_MAX:
> 
> 		if ((id >= MAX_IDR_BIT) || (id < 0))
> 			return -ENOSPC;
> 
> These two conditions are actually the same for "int" variable so one
> of them can be removed. If the above snippet is compiled with -Os option
> of gcc, only one checking will remain in disassembly code.
> 
> --- a/lib/idr.c
> +++ b/lib/idr.c
> @@ -262,7 +262,7 @@ static int sub_alloc(struct idr *idp, int *starting_id, struct idr_layer **pa,
>  			sh = IDR_BITS*l;
>  			id = ((id >> sh) ^ n ^ m) << sh;
>  		}
> -		if ((id >= MAX_IDR_BIT) || (id < 0))
> +		if (id >= MAX_IDR_BIT)
>  			return -ENOSPC;
>  		if (l == 0)
>  			break;

Well.  This only works because MAX_IDR_BIT happens to have unsigned
type, so the comparison is done with unsigned arithmetic.

The patch makes no difference to code size with my gcc and I'm inclined
to leave the code as-is for reasons of safety and clarity.

Re: [PATCH] lib/idr.c: remove duplicated bound checking in sub_alloc

[Liu Sha]

On Sat, Mar 21, 2015 at 5:28 AM, Andrew Morton
<akpm@linux-foundation.org> wrote:
> On Thu, 19 Mar 2015 17:57:07 +0000 Liu Sha <rednoax@qq.com> wrote:
>
>> From: Liu Sha <rednoax@gmail.com>
>>
>> The INT_MAX bound checking in sub_alloc checks two conditions to see
>> whether the signed integer "id" is beyond INT_MAX:
>>
>>               if ((id >= MAX_IDR_BIT) || (id < 0))
>>                       return -ENOSPC;
>>
>> These two conditions are actually the same for "int" variable so one
>> of them can be removed. If the above snippet is compiled with -Os option
>> of gcc, only one checking will remain in disassembly code.
>>
>> --- a/lib/idr.c
>> +++ b/lib/idr.c
>> @@ -262,7 +262,7 @@ static int sub_alloc(struct idr *idp, int *starting_id, struct idr_layer **pa,
>>                       sh = IDR_BITS*l;
>>                       id = ((id >> sh) ^ n ^ m) << sh;
>>               }
>> -             if ((id >= MAX_IDR_BIT) || (id < 0))
>> +             if (id >= MAX_IDR_BIT)
>>                       return -ENOSPC;
>>               if (l == 0)
>>                       break;
>
> Well.  This only works because MAX_IDR_BIT happens to have unsigned
> type, so the comparison is done with unsigned arithmetic.
>
> The patch makes no difference to code size with my gcc and I'm inclined
> to leave the code as-is for reasons of safety and clarity.
Thanks for your explaination. Because address "qq.com" of this mail is
among the "not liked source for email" of LKML and has been returned. I
wrote 2nd mail with the same patch using another email address. You may
have received two similar mail. Please ignore the 2nd.

I think it may be better to shrink this checking for the following reasons :)

1. two equivalent conditions is a little confusing. When I first see it, I know
what "id < 0" means but not 100% sure about "id > MAX_IDR_BIT". "id" is
signed while "MAX_IDR_BIT" is unsigned. So after test I belive it becomes
an unsigned comparsion, as you said. And its function is equivalent to "id < 0".
I tried two c files, using "id > MAX_IDR_BIT" and "id < 0" respectively, gcc
will interpreter both as "id < 0". So just one condition will be much clear to
let people know that it only want to filter "id < 0".

2. The top Makefile specifies either "-Os" or "-O2" for gcc. So there is not
any difference in generated assembly code in idr.lst, no matter which one of
these two conditions is removed.  The valid idr range is [0, INT_MAX], which
has been so since 2.6.12. There are some other places in idr.c checking INT_MAX.
They either use "> MAX_IDR_BIT" or "< 0", but not both. So only one is safe
enough.
--
Liu Sha