On 10/23/20 9:02 AM, Jeroen Roovers wrote:
> On Thu, 22 Oct 2020 22:29:18 +0200
> Helge Deller <deller@xxxxxx> wrote:
>
>> On 10/22/20 9:11 PM, Meelis Roos wrote:
>>>
>>> 22.10.20 19:40 Helge Deller wrotw:
>>>> This patch adds wrapper functions for the relevant syscalls. The
>>>> wrappers masks out any old invalid O_NONBLOCK flags, reports in the
>>>> syslog if the old O_NONBLOCK value was used and then calls the
>>>> target syscall with the new O_NONBLOCK value.
>>>>
>>>> Fixes: 75ae04206a4d ("parisc: Define O_NONBLOCK to become
>>>> 000200000") Signed-off-by: Helge Deller <deller@xxxxxx>
>>>
>>> Works for me on RP2470 - boots up in full functionality and logs
>>> the recompilation warning about systemd-udevd and syslog-ng.
>>>
>>> Tested-by: Meelis Roos <mroos@xxxxxxxx>
>>
>> Thank you Meelis & Jeroen for testing!
>>
>> The big question is:
>> We have two options
>> a) revert my original commit 75ae04206a4d ("parisc: Define O_NONBLOCK
>> to become 000200000"), or b) apply and submit this new patch on top
>> of it.
>>
>> The benefit in b) is that we will get long term rid of the two-bit
>> O_NONBLOCK define and thus will get more compatible to other Linux
>> architectures. This comes with the downside of (at least for a few
>> years) added overhead for those non-performance critical syscalls.
>
> The performance issue is resolved once the installed kernel
> headers/libc have been updated accordingly. I think after that the
> overhead should be minimal.
Yes, probably.
>> The benefit with a) is that we then step back again and stay
>> compatible now. The downside is, that in the future we may run into
>> other issues and need to keep special-handling in some other syscalls
>> forever.
>>
>> I'm still for going with b), and I hope we got all corner cases ruled
I meant "proposing to go with option b)"....
>> out. Opinions?
>
> I think the performance penalty isn't that great, but could be
> improved, so I'd go for b) with a small change. The warning that it
> issues seems redundant, because the immediate problem has already been
> "solved", and because the proposed solution does not work:
>
> +{
> + if (flags & O_NONBLOCK_MASK_OUT) {
> + struct task_struct *tsk = current;
> + pr_warn("%s(%d) uses old O_NONBLOCK value. "
> + "Please recompile the application.\n",
> + tsk->comm, tsk->pid);
> + }
> + return flags & ~O_NONBLOCK_MASK_OUT;
> +}
> +
>
> The text assumes that the officially packaged kernel headers are in
> sync with the kernel, which normally isn't the case as most
> distributions seem to either pick an LTS kernel for their toolchain, or
> keep the installed kernel in sync with the installed kernel headers,
> but do not prevent running newer kernels and may even encourage doing
> so. Simply recompiling the programs that use the old O_NONBLOCK value
> does not solve the problem in most cases.
True.
> If you'd remove that if() { pr_warn } entirely, you'd probably be
> rid of most of the performance penalty anyway.
I think it makes sense to keep at least one warning.
Rolf Eike proposed a pr_warn_once() which seems the best compromise
as it keeps us at least informed which packages needs updating and
how relevant it is to keep those wrappers....
Helge
