Bug 192842 - [patch][linux] linux_ppoll syscall required by linux apps that use pulseaudio (on linux_base-f20)
Summary: [patch][linux] linux_ppoll syscall required by linux apps that use pulseaudio...
Status: Closed FIXED
Alias: None
Product: Base System
Classification: Unclassified
Component: kern (show other bugs)
Version: CURRENT
Hardware: amd64 Any
: --- Affects Only Me
Assignee: Dmitry Chagin
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-08-20 00:10 UTC by Vassilis Laganakos
Modified: 2016-01-09 21:31 UTC (History)
7 users (show)

See Also:


Attachments
linux_ppoll syscall patch for linuxulator (3.58 KB, patch)
2014-08-20 00:10 UTC, Vassilis Laganakos
no flags Details | Diff
linux_ppoll syscall patch for linuxulator v2 (3.90 KB, patch)
2014-09-04 18:52 UTC, Vassilis Laganakos
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Vassilis Laganakos 2014-08-20 00:10:41 UTC
Created attachment 146055 [details]
linux_ppoll syscall patch for linuxulator

While testing the linux_base-f20 and rest of -f20 ports, I was trying to get Skype to work with pulseaudio on the native side. For that to work seems that it needs at least 2 more syscalls in the linuxulator, fstatfs64 and ppoll. The latter one is most likely required by any linux app that uses pulse audio.

So this is a naive implementation of linux_ppoll, tested to work with some pulseaudio command line utils (e.g. /compat/linux/usr/bin/paplay) playing correctly a few wav files. The machine is amd64 running lemul (which is a couple of weeks behind CURRENT but the patch was ported). I followed the guidelines from man (2) ppoll from Linux. I don't know if there is a more pretty way to do the save and restore the sigmask, so if there is please let me know so I can update this.

I have done some basic testing of the sigmask part of linux_ppoll (e.g. blocking SIGHUP whille polling) and that does seem to work, but not something extansive. paplay seems to pass NULL in its ppoll call, so it wasn't useful for testing this part.
Comment 1 Pedro F. Giffuni freebsd_committer 2014-08-23 18:02:06 UTC
Assign to maintainer.
Comment 2 Jilles Tjoelker freebsd_committer 2014-08-31 14:45:27 UTC
Some comments about the code (I have not tested this in any way):

* Look at pselect() for how to handle the signal mask. The idea of the signal mask parameter is to unblock some signals atomically with starting to wait for events on file descriptors. I think the way you are doing it, an unblocked signal will interrupt ppoll(), but will not call the signal handler.

* The variable pap is not needed, you can use &pa where it is used.

* Per style(9), the variable pa should be declared at the top of the function.

* The less accurate timeout (ms) is probably acceptable for an emulation call like this, but it should be fixed if and when a native ppoll() is added. An overflow check may be appropriate, though: the multiplication tvp->tv_sec * 1000 may overflow and so may the addition to the sub-second part. The special case for 0 seconds is not needed.
Comment 3 Vassilis Laganakos 2014-09-04 18:52:46 UTC
Created attachment 146818 [details]
linux_ppoll syscall patch for linuxulator v2

Thank you for taking a look in this.

You're right about the pointer to pa variable; I've dropped it and move the declaration of pa early in the function.

Switched back to using ast() as in linux_pselect, dropped the 0s case, and added handling the multiplication and addition potential overflows in a naive way: if the result is smaller than the multiplicator or then the addend, then timeout will get the INT_MAX value.
Comment 4 Steve Wills freebsd_committer 2014-10-09 21:35:12 UTC
What's the status of this?
Comment 5 Vassilis Laganakos 2014-10-12 11:40:16 UTC
Hello. I don't know if there is anything else that I should do, so I'm just wating to see what happens or if anyone asks for something :) The basic testing has been done on the lemul branch. What are the next steps for this to go in tree?
Comment 6 Mateusz Guzik freebsd_committer 2014-10-12 13:47:56 UTC
+linux_ppoll(struct thread *td, struct linux_ppoll_args *pargs)
+{
+	struct l_timespec lts;
+	struct timeval tv, *tvp, tv0, tv1;
+	struct poll_args pa;
+	sigset_t usigmask, *usigmaskp;
+	l_sigset_t lsigmask;
+	int error, retval, mss, msns;

error and retval?

+
+	/* Get the time */
+	if (pargs->timeout_ts != NULL)
+	{

style

+		error = copyin(pargs->timeout_ts, &lts, sizeof(lts));
+		if (error != 0)
+			return (error);
+
+		TIMESPEC_TO_TIMEVAL(&tv, &lts);
+		if (itimerfix(&tv))
+			return (EINVAL);
+
+		/* Mark the time before the call */
+		microtime(&tv0);
+		tvp = &tv;
+	} else
+		tvp = NULL;
+
+	/* Get the sigmask */
+	if (pargs->sigmask != NULL) {
+		error = copyin(pargs->sigmask, &lsigmask, sizeof(lsigmask));
+		if (error)
+			return (error);
+		linux_to_bsd_sigset(&lsigmask, &usigmask);
+		usigmaskp = &usigmask;
+	} else
+		usigmaskp = NULL;
+
+	/* Set the sigmask */
+	error = kern_sigprocmask (td, SIG_SETMASK, usigmaskp, &td->td_oldsigmask, 0);
+	if (error)
+		return (error);
+
+	pa.fds = pargs->fds;
+	pa.nfds = pargs->nfds;
+
+	/* Linux's ppoll allows NULL timeout which is translated to FreeBSD's INFTIM (-1) */
+	if (tvp==NULL)
+		pa.timeout = INFTIM;
+	else
+	{

style

+		mss = tvp->tv_sec * 1000;
+		msns = tvp->tv_usec / 1000;
+
+		/*
+		 * Handling the multiplication and addition overflow.
+		 * If it happens, assing pa.timeout the INT_MAX value
+		 */
+		if (mss/1000 == tvp->tv_sec) {
+			pa.timeout = mss + msns;
+			if ( pa.timeout < 0)
+				pa.timeout = INT_MAX;
+		} else
+			pa.timeout = INT_MAX;
+	}
+
+	/*
+	* Ensure that the td_oldsigmask is restored by ast()
+	* when returns to user mode and that the TDP_OLDMASK
+	* is cleared
+	*/
+	td->td_pflags |= TDP_OLDMASK;
+	thread_lock(td);
+	td->td_flags |= TDF_ASTPENDING;
+	thread_unlock(td);

Why was not this done immediately after kern_sigprocmask?

Also, this would benefit from kern_ppoll helper.

Given that pselect has similar code, maybe an additional helper should be introduced.

+
+	/* call sys_poll */
+	retval = sys_poll(td, &pa);
+
+	if (retval == 0 && pargs->timeout_ts) {
+		if (td->td_retval[0]) {
+		/*
+		* Compute how much time was left of the timeout,
+		* by subtracting the current time and the time
+		* before we started the call, and subtracting
+		* that result from the user-supplied value.
+		*/
+			microtime(&tv1);
+			timevalsub(&tv1, &tv0);
+			timevalsub(&tv, &tv1);
+			if (tv.tv_sec < 0)
+				timevalclear(&tv);
+		} else
+			timevalclear(&tv);
+
+			TIMEVAL_TO_TIMESPEC(&tv, &lts);
+
+		error = copyout(&lts, pargs->timeout_ts, sizeof(lts));
+		if (error)
+			return (error);
+	}
+	return retval;
+}
+
+int
Comment 7 Edward Tomasz Napierala freebsd_committer 2014-10-13 07:29:32 UTC
+1 for kern_ppoll().  It would also be great if it was available for native FreeBSD applications.
Comment 8 Mateusz Guzik freebsd_committer 2014-10-14 17:50:28 UTC
Further look reveals both select and poll require similar code related to timers, so this can be moved into a helper as well.

Also these patches have numerous formatting issues not mentioned earlier in my previous comment.

What was used to test correctness? At least ltp's tests should be used, and maybe ones from glibc (if they have some for ppoll that is). Not everything has to pass, but getting an idea what's up is definitely helpful
Comment 9 Ruslan Makhmatkhanov freebsd_committer 2014-10-27 05:38:39 UTC
Hi,

I tried the second revision of this patch on -current r273290 and it doesn't builds:

cc  -O2 -pipe  -DCOMPAT_FREEBSD32 -DCOMPAT_LINUX32 -fno-strict-aliasing -Werror -D_KERNEL -DKLD_MODULE -nostdinc   -DHAVE_KERNEL_OPTION_HEADERS -include /mnt/media/d1/obj/mnt/media/d1/src/sys/Z1/opt_global.h -I. -I@ -I@/contrib/altq -fno-common  -fno-omit-frame-pointer -mno-omit-leaf-frame-pointer -I/mnt/media/d1/obj/mnt/media/d1/src/sys/Z1  -mcmodel=kernel -mno-red-zone -mno-mmx -mno-sse -msoft-float  -fno-asynchronous-unwind-tables -ffreestanding -fstack-protector -mno-aes -mno-avx -Qunused-arguments -std=iso9899:1999 -fstack-protector -Wall -Wredundant-decls -Wnested-externs -Wstrict-prototypes  -Wmissing-prototypes -Wpointer-arith -Winline -Wcast-qual  -Wundef -Wno-pointer-sign -fformat-extensions  -Wmissing-include-dirs -fdiagnostics-show-option  -Wno-error-tautological-compare -Wno-error-empty-body  -Wno-error-parentheses-equality -Wno-error-unused-function  -mno-aes -mno-avx -Qunused-arguments -c /mnt/media/d1/src/sys/modules/linux/../../compat/linux/linux_misc.c
/mnt/media/d1/src/sys/modules/linux/../../compat/linux/linux_misc.c:581:13: error: no member named 'timeout_ts' in 'struct linux_ppoll_args'
        if (pargs->timeout_ts != NULL)
            ~~~~~  ^
/mnt/media/d1/src/sys/modules/linux/../../compat/linux/linux_misc.c:583:25: error: no member named 'timeout_ts' in 'struct linux_ppoll_args'
                error = copyin(pargs->timeout_ts, &lts, sizeof(lts));
                               ~~~~~  ^
/mnt/media/d1/src/sys/modules/linux/../../compat/linux/linux_misc.c:598:13: error: no member named 'sigmask' in 'struct linux_ppoll_args'
        if (pargs->sigmask != NULL) {
            ~~~~~  ^
/mnt/media/d1/src/sys/modules/linux/../../compat/linux/linux_misc.c:599:25: error: no member named 'sigmask' in 'struct linux_ppoll_args'
                error = copyin(pargs->sigmask, &lsigmask, sizeof(lsigmask));
                               ~~~~~  ^
/mnt/media/d1/src/sys/modules/linux/../../compat/linux/linux_misc.c:612:18: error: no member named 'fds' in 'struct linux_ppoll_args'
        pa.fds = pargs->fds;
                 ~~~~~  ^
/mnt/media/d1/src/sys/modules/linux/../../compat/linux/linux_misc.c:613:19: error: no member named 'nfds' in 'struct linux_ppoll_args'
        pa.nfds = pargs->nfds;
                  ~~~~~  ^
/mnt/media/d1/src/sys/modules/linux/../../compat/linux/linux_misc.c:648:28: error: no member named 'timeout_ts' in 'struct linux_ppoll_args'
        if (retval == 0 && pargs->timeout_ts) {
                           ~~~~~  ^
/mnt/media/d1/src/sys/modules/linux/../../compat/linux/linux_misc.c:666:32: error: no member named 'timeout_ts' in 'struct linux_ppoll_args'
                error = copyout(&lts, pargs->timeout_ts, sizeof(lts));
                                      ~~~~~  ^
8 errors generated.
*** Error code 1
Comment 10 Dmitry Chagin freebsd_committer 2014-11-05 07:56:59 UTC
https://reviews.freebsd.org/D1104
https://reviews.freebsd.org/D1105

can be applied to the lemul branch
Comment 11 Dmitry Chagin freebsd_committer 2015-05-24 21:23:05 UTC
grab
Comment 12 Dmitry Chagin freebsd_committer 2016-01-09 21:31:27 UTC
in stable/10 now