Messages in this thread | | | From | Liu Sha <> | Date | Sat, 21 Mar 2015 15:47:08 +0800 | Subject | Re: [PATCH] lib/idr.c: remove duplicated bound checking in sub_alloc |
| |
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
| |