search

mikaku / Fiwix

A UNIX-like kernel for the i386 architecture
https://www.fiwix.org
Other
401 stars 32 forks source link

Support stub for syscall madvise #55

Closed rick-masters closed 7 months ago

rick-masters commented 7 months ago

This feature request is to support a stub system call for madvise that always returns success.

madvise is a system call to "advise" the kernel about how you intend to use memory and is intended to increase performance.

Commonly, an application will operate correctly even if the madvise system call is ignored by the kernel. However, there are many options available and I haven't reviewed all of them and so it may not be generally a good idea to return success unless the call is actually implemented. Therefore, I have implemented this behind a config option which is disabled by default.

Note that the definition for SYS_madvise in unistd.h is not behind the config option. That's because it didn't seem like an application should require a special config option to access the SYS_madvise constant. Maybe an application or library would like to use that constant to make the system call and can handle it being unimplemented. Anyway, it doesn't matter to me if you want to wrap that with an #ifdef instead.

The reason this stub is necessary is that Live-bootstrap starts with an older version of musl (1.1.24) which calls madvise internally (in some malloc related functions) to advise the kernel it is no longer using memory and unfortunately leaves the errno variable set to an error if madvise returns an error due to not being implemented. This nonzero errno propagates to the application which can fail as a result. In the case of live-bootstrap I believe the find program is failing as part of building the linux kernel package. (It appears that a later version of musl (1.2.4) doesn't let the result of madvise affect the errno but live-bootstrap is using musl-1.1.24 at the beginning because it is simpler to build.)

I have not included a test program because there is no new functionality to test other than the return code, which is obvious from the code and also I have tested the forthcoming PR in live-bootstrap for months.

mikaku commented 7 months ago

Probably you already thought on this but, it wouldn't be easier to patch musl to ignore this system call (i.e. by commenting the line) and returning zero?

rick-masters commented 7 months ago

Probably you already thought on this but, it wouldn't be easier to patch musl to ignore this system call (i.e. by commenting the line) and returning zero?

Actually this might be a better solution. It's been a while since I tracked this down in musl so it might take some time to verify the patch works. You can hold off integrating the PR for now.

rick-masters commented 7 months ago

Ok, patching musl to preserve errno around the madvise call worked. This was really a bug in musl that was fixed in a later version so it makes sense to patch musl in this case.

I was in the habit of addressing the issue on the Fiwix side because nearly all the issues I have run into like this were primarily due to an unimplemented syscall in Fiwix but this one is primarily a bug in musl for not ignoring the unimplemented syscall.

I'm going to close this Issue and the PR.

mikaku commented 7 months ago

I'm really glad that this time the patch was in the musl side :-). Thank you very much for taking care of this.