diff mbox series

[liburing,v1,2/9] setup: Handle `get_page_size()` failure (for aarch64 nolibc support)

Message ID 20220629002028.1232579-3-ammar.faizi@intel.com (mailing list archive)
State New
Headers show
Series aarch64 nolibc support | expand

Commit Message

Ammar Faizi June 29, 2022, 12:27 a.m. UTC 
From: Ammar Faizi <ammarfaizi2@gnuweeb.org>

This is a preparation patch to add aarch64 nolibc support.

aarch64 supports three values of page size: 4K, 16K, and 64K which are
selected at kernel compilation time. Therefore, we can't hard code the
page size for this arch. We will utilize open(), read() and close()
syscall to find the page size from /proc/self/auxv. Since syscall may
fail, we may also fail to get the page size here. Handle the failure.

Signed-off-by: Ammar Faizi <ammarfaizi2@gnuweeb.org>
---
 src/setup.c | 3 +++
 1 file changed, 3 insertions(+)

Comments

Alviro Iskandar Setiawan June 29, 2022, 12:50 a.m. UTC | #1 
On Wed, Jun 29, 2022 at 7:28 AM Ammar Faizi wrote:
>         page_size = get_page_size();
> +       if (page_size < 0)
> +               return page_size;
> +
>         return rings_size(p, entries, cq_entries, page_size);
>  }

the current error handling fallback to 4K if fail on sysconf(_SC_PAGESIZE):
https://github.com/axboe/liburing/blob/68103b731c34a9f83c181cb33eb424f46f3dcb94/src/arch/generic/lib.h#L10-L19
with this patch, get_page_size() is only possible to return negative
value on aarch64.

i don't understand why the current master branch code fallback to 4K when fail?

tq

-- Viro
Ammar Faizi June 29, 2022, 1 a.m. UTC | #2 
On 6/29/22 7:50 AM, Alviro Iskandar Setiawan wrote:
> On Wed, Jun 29, 2022 at 7:28 AM Ammar Faizi wrote:
>>          page_size = get_page_size();
>> +       if (page_size < 0)
>> +               return page_size;
>> +
>>          return rings_size(p, entries, cq_entries, page_size);
>>   }
> 
> the current error handling fallback to 4K if fail on sysconf(_SC_PAGESIZE):
> https://github.com/axboe/liburing/blob/68103b731c34a9f83c181cb33eb424f46f3dcb94/src/arch/generic/lib.h#L10-L19
> with this patch, get_page_size() is only possible to return negative
> value on aarch64.

Ah right, this one needs a revision. Either we fallback to 4K, or
return error if we fail.

> i don't understand why the current master branch code fallback to 4K when fail?

Neither do I. Maybe because 4K is widely used page size?

Jens, can you shed some light on this?

   The current upstream does this:

      - For x86/x86-64, it's hard-coded to 4K. So it can't fail.
      - For other archs, if sysconf(_SC_PAGESIZE) fails, we fallback to 4K.

Now we are going to add aarch64, it uses a group of syscalls to get the page
size. So it may fail. What should we do when we fail?

Fallback to 4K? Or return error code from syscall?
Jens Axboe June 29, 2022, 3 p.m. UTC | #3 
On 6/28/22 7:00 PM, Ammar Faizi wrote:
> On 6/29/22 7:50 AM, Alviro Iskandar Setiawan wrote:
>> On Wed, Jun 29, 2022 at 7:28 AM Ammar Faizi wrote:
>>>          page_size = get_page_size();
>>> +       if (page_size < 0)
>>> +               return page_size;
>>> +
>>>          return rings_size(p, entries, cq_entries, page_size);
>>>   }
>>
>> the current error handling fallback to 4K if fail on sysconf(_SC_PAGESIZE):
>> https://github.com/axboe/liburing/blob/68103b731c34a9f83c181cb33eb424f46f3dcb94/src/arch/generic/lib.h#L10-L19
>> with this patch, get_page_size() is only possible to return negative
>> value on aarch64.
> 
> Ah right, this one needs a revision. Either we fallback to 4K, or
> return error if we fail.
> 
>> i don't understand why the current master branch code fallback to 4K when fail?
> 
> Neither do I. Maybe because 4K is widely used page size?
> 
> Jens, can you shed some light on this?
> 
>   The current upstream does this:
> 
>      - For x86/x86-64, it's hard-coded to 4K. So it can't fail.
>      - For other archs, if sysconf(_SC_PAGESIZE) fails, we fallback to 4K.
> 
> Now we are going to add aarch64, it uses a group of syscalls to get the page
> size. So it may fail. What should we do when we fail?
> 
> Fallback to 4K? Or return error code from syscall?

4k is the most common page size, by far, so makes sense to have that as
a fallback rather than just error out. Perhaps the application will then
fail differently, but there's also a chance that it'll just work.

So I think just falling back to 4k if we fail for whatever reason is the
sanest recourse.
Ammar Faizi June 29, 2022, 3:17 p.m. UTC | #4 
On 6/29/22 10:00 PM, Jens Axboe wrote:
> 4k is the most common page size, by far, so makes sense to have that as
> a fallback rather than just error out. Perhaps the application will then
> fail differently, but there's also a chance that it'll just work.
> 
> So I think just falling back to 4k if we fail for whatever reason is the
> sanest recourse.

OK, I'll do that in the v2 revision.
diff mbox series

Patch

diff --git a/src/setup.c b/src/setup.c
index d2adc7f..ca9d30d 100644
--- a/src/setup.c
+++ b/src/setup.c
@@ -336,6 +336,9 @@  ssize_t io_uring_mlock_size_params(unsigned entries, struct io_uring_params *p)
 	}
 
 	page_size = get_page_size();
+	if (page_size < 0)
+		return page_size;
+
 	return rings_size(p, entries, cq_entries, page_size);
 }