Re: [PATCH liburing 0/3] Add x86 32-bit support for the nolibc build

[Ammar Faizi <ammarfaizi2@gnuweeb.org>]

On 4/18/22 9:01 AM, Jens Axboe wrote:
> On 4/14/22 4:41 PM, Ammar Faizi wrote:
>> Hi,
>>
>> This series adds nolibc support for x86 32-bit. There are 3 patches in
>> this series:
>>
>> 1) Use `__NR_mmap2` instead of `__NR_mmap` for x86 32-bit.
>> 2) Provide `get_page_size()` function for x86 32-bit.
>> 3) Add x86 32-bit native syscall support.
>>
>> The most noticeable changes is the patch 3. Unlike x86-64, only
>> use native syscall from the __do_syscall macros when CONFIG_NOLIBC is
>> enabled for 32-bit build. The reason is because the libc syscall
>> wrapper can do better in 32-bit. The libc syscall wrapper can dispatch
>> the best syscall instruction that the environment is supported, there
>> are at least two variants syscall instruction for x86 32-bit, they are:
>> `int $0x80` and `sysenter`. The `int $0x80` instruction is always
>> available, but `sysenter` is not, it relies on VDSO. liburing always
>> uses `int $0x80` for syscall if it's compiled with CONFIG_NOLIBC,
>> otherwise it uses whatever the libc provides.
>>
>> Extra notes for __do_syscall6() macro:
>> On i386, the 6th argument of syscall goes in %ebp. However, both Clang
>> and GCC cannot use %ebp in the clobber list and in the "r" constraint
>> without using -fomit-frame-pointer. To make it always available for
>> any kind of compilation, the below workaround is implemented:
>>
>>    1) Push the 6-th argument.
>>    2) Push %ebp.
>>    3) Load the 6-th argument from 4(%esp) to %ebp.
>>    4) Do the syscall (int $0x80).
>>    5) Pop %ebp (restore the old value of %ebp).
>>    6) Add %esp by 4 (undo the stack pointer).
>>
>> WARNING:
>>    Don't use register variables for __do_syscall6(), there is a known
>>    GCC bug that results in an endless loop.
>>
>> BugLink: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105032
>>
>>
>> ===== How is this tested? =====
>>
>> This has been tested on x86-64 Linux (compiled with 32-bit bin support)
>> with the following commands:
>>
>>    sudo apt-get install gcc-i686-linux-gnu g++-i686-linux-gnu -y;
>>    ./configure --cc=i686-linux-gnu-gcc --cxx=i686-linux-gnu-g++ --nolibc;
>>    sudo make -j8 runtests;
> 
> Looks reasonable to me, even with the warts. I keep threatening to do a
> 2.2 release, and I do want to do that soon, so question is if we defer
> this patchset until after that has happened?
> 
> I'm looking for a gauge of confidence on the series.

I personally love not to defer this patchset. I understand that if we
were adding something like this to the Linux kernel, it's pretty sure
that it is not acceptable time. But liburing.

Several things that you may want to consider:

1) Previously, `--nolibc` build on x86 32-bit will throw a compile
    error, "Arch doesn't support building liburing without libc".
    After this patchset, it compiles just fine.

2) This series doesn't have any effect for x86 32-bit that uses libc,
    and that is what we do by default.

3) I believe x86 32-bit users are not that many. So having this one
    earlier gives sometime to get it mature without much chaos (if
    we ever found a bug).

    Not to say it's buggy. But younger code tend to be buggier. If we
    ever hit that bug due to this patchset, some of them may fallback
    to the libc build while waiting for the next stable liburing.

But anyway, I don't think it's that urgent seeing that we don't have
visible users that are actively using nolibc x86 32-bit. So if you
prefer to defer this, please defer it. What do you think?

-- 
Ammar Faizi