Re: [RFC PATCH] parisc: Add wrapper syscalls to fix O_NONBLOCK flag usage

[Jeroen Roovers <jer@xs4all.nl>]

On Thu, 22 Oct 2020 22:29:18 +0200
Helge Deller <deller@gmx.de> 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@gmx.de>  
> >
> > 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@linux.ee>  
> 
> 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.

> 
> 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
> 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.

If you'd remove that if() { pr_warn } entirely, you'd probably be
rid of most of the performance penalty anyway.


Regards,
     jer