Hi,<br><br>The performance hit with getpw* is getting much worse when you have a remote database (e.g ldap).<br><br>You may consider checking the range first as this does not require syscall, and then checking the user.<br><br>Kind regards,<br>-- <br>Nekral<br><br><br>Le samedi 6 septembre 2014, <<a href="mailto:bostjan@a2o.si">bostjan@a2o.si</a>> a écrit :<br>> Heh, I just retested this with 1 000 000 entries, but this time<br>> entries were specified with UID instead of username. Result: 0.310s!<br>><br>> It seems that getpwnam() returns very quickly if you pass it UID<br>> (string) instread of actual username.<br>><br>> Anyway, I have added a note to subuid/subgid man pages, pushed to -v3<br>> branch this commit too.<br>><br>> b.<br>><br>><br>><br>><br>> On 6 September 2014 00:14, Bostjan Skufca <<a href="mailto:bostjan@a2o.si">bostjan@a2o.si</a>> wrote:<br>>> Also, I  ran some benchmarks of the old and new code, under the<br>>> following conditions:<br>>> - modest i3 3.3GHz machine, fairly unloaded<br>>> - 1.000.000 entries in /etc/subuid<br>>> - command: time ./src/newuidmap 8260 0 100000 100<br>>> - command fails as there is no appropriate entry in /etc/subuid<br>>> - results repeated several times, average taken<br>>><br>>> Here are the results(n=1 000 000):<br>>> - 0.290s: Single loop, no syscalls (old behaviour)<br>>> - 0.320s: Double loop, no syscalls (new behaviour, but disabled all<br>>> syscalls in the second loop, temporarily)<br>>> - 7.500s: Double loop, with getpwnam() syscalls in second loop<br>>><br>>> This last entry should concern us, but I think that people who intend<br>>> to put 1M records into /etc/subuid, will gladly write their own tools<br>>> to optimize this.<br>>><br>>> I rerun the test, with single loop without syscalls, and varied the<br>>> number of entries in /etc/subuid. Results:<br>>> - 0.001s: with      1000 entries<br>>> - 0.004s: with    10 000 entries<br>>> - 0.032s: with   100 000 entries<br>>> - 0.290s: with 1 000 000 entries<br>>><br>>> Double loop without syscalls, and varied the number of entries in<br>>> /etc/subuid. Results:<br>>> - 0.001s: with      1000 entries<br>>> - 0.004s: with    10 000 entries<br>>> - 0.033s: with   100 000 entries<br>>> - 0.300s: with 1 000 000 entries<br>>><br>>> Obviously, number of loops does not matter all that much if no<br>>> syscalls are used. Also, second loop is WAY faster than the first one.<br>>><br>>> Last test I did was with the final patch applied as-is. Double loop,<br>>> with syscalls in the second loop. Results:<br>>> - 0.009s: with      1000 entries<br>>> - 0.075s: with    10 000 entries<br>>> - 0.740s: with   100 000 entries<br>>> - 7.500s: with 1 000 000 entries<br>>><br>>> Realistically I believe that people will max this implementation at<br>>> around 100-500 entries in the /etc/sub[ug]id. I think that this<br>>> implementation performs adequately in that area.<br>>> (I do hope I do NOT get famous for "500 entries in subuid ought to be<br>>> enough for everybody":)<br>>><br>>><br>>> b.<br>>><br>>> On 5 September 2014 23:53, Bostjan Skufca <<a href="mailto:bostjan@a2o.si">bostjan@a2o.si</a>> wrote:<br>>>>>> I created new branch for this one, for you to merge one single commig:<br>>>>>> <a href="https://github.com/bostjan/shadow/commits/feature/subuidgid-numeric-v2">https://github.com/bostjan/shadow/commits/feature/subuidgid-numeric-v2</a><br>>>>><br>>>>> My 2c:<br>>>>> Do you need to include <grp.h>?<br>>>><br>>>> Leftovers. When I started looking into this I was under impression<br>>>> that uidmap is determined by user's UID and gidmap is determined by<br>>>> user's GID. It turned out getgwnam() was not needed at all, as<br>>>> everything is UID-based.<br>>>><br>>>><br>>>>> You could avoid the getpwnam_r call (and malloc, sysconf) by calling<br>>>>> getpwnam. You just need to store owner_pwd.pw_uid before you enter the<br>>>>> loop.<br>>>>> This would also avoid the call to exit(), which IMO should be avoided in<br>>>>> commonio.c (better return an error code and let the applications deal with<br>>>>> the error - this can let them release a lock for example).<br>>>><br>>>> So obvious :)<br>>>> (I copy-pasted the code strainght from getpwnam man page, that is why<br>>>> exit()s were there instead of proper error handling.)<br>>>><br>>>> Unfortunately there is not a single instance of error handling in<br>>>> lib/subordinateio.c to take as example, and I am not sure that calling<br>>>> code knows how to handle errors other than "requested uidmap not<br>>>> allowed", which is what "return NULL;" stands for.<br>>>><br>>>> Therefore ideas about how to do proper error handling are very<br>>>> welcome, but I think they belong to a separate thread.<br>>>><br>>>><br>>>>> I'm not sure regarding the guarantees you have on the size of pw_uid.<br>>>>> I would recommend:<br>>>>>         sprintf(owner_uid_string, "%lu", (unsigned long int)owner_pwd.pw_uid);<br>>>><br>>>> Sensible, fixed.<br>>>><br>>>><br>>>>> This is rather cosmetic, but<br>>>>>                         if (!range_owner_pwd) {<br>>>>> could be changed to:<br>>>>>                         if (NULL == range_owner_pwd) {<br>>>><br>>>> Future-proofing of course, fixed that too.<br>>>><br>>>><br>>>> Commit with fixes to review:<br>>>> <a href="https://github.com/bostjan/shadow/commit/d21214c9ba823edd5ac82ae2b1b03f4c219c3841">https://github.com/bostjan/shadow/commit/d21214c9ba823edd5ac82ae2b1b03f4c219c3841</a><br>>>><br>>>> Branch to actually merge (contains single commit with everything<br>>>> squashed into it):<br>>>> <a href="https://github.com/bostjan/shadow/tree/feature/subuidgid-numeric-v3">https://github.com/bostjan/shadow/tree/feature/subuidgid-numeric-v3</a><br>>>><br>>>><br>>>> @Serge:<br>>>> Maybe it would be for the best if you coordinated with Christian<br>>>> inclusion of this patch into upstream, as you are already "in the<br>>>> upstream-patch-pushing zone" :)<br>>>><br>>>><br>>>> b.<br>><br><br>-- <br>Nekral<br>