Re: [PATCH] ipc/msg: add msgsnd_timed and msgrcv_timed syscall for system V message queue
Hi Eric,
On 3/4/21 2:12 AM, Andrew Morton wrote:
On Tue, 23 Feb 2021 23:11:43 +0800 Eric Gao wrote:
sometimes, we need the msgsnd or msgrcv syscall can return after a limited
time, so that the business thread do not be blocked here all the time. In
this case, I add the msgsnd_timed and msgrcv_timed syscall that with time
parameter, which has a unit of ms.
Please cc Manfred and Davidlohr on ipc/ changes.
The above is a very brief description for a new syscall! Please go to
great lengths to tell us why this is considered useful - what are the
use cases?
Also, please fully describe the proposed syscall interface right here
in the changelog. Please be prepared to later prepare a full manpage.
...
+SYSCALL_DEFINE5(msgsnd_timed, int, msqid, struct msgbuf __user *, msgp,
size_t, msgsz,
+ int, msgflg, long, timeoutms)
Specifying the timeout in milliseconds is problematic - it's very
coarse. See sys_epoll_pwait2()'s use of timespecs.
What about using an absolute timeout, like in mq_timedsend()?
That makes restart handling after signals far simpler.
> - schedule();
> +
> + /* sometimes, we need msgsnd syscall return after a given
time */
> + if (timeoutms <= 0) {
> + schedule();
> + } else {
> + timeoutms = schedule_timeout(timeoutms);
> + if (timeoutms == 0)
> + timeoutflag = true;
> + }
I wonder if this should be schedule_timeout_interruptible() or at least
schedule_timeout_killable() instead of schedule_timeout(). If it should,
this should probably be done as a separate change.
No. schedule_timeout_interruptible() just means that
__set_current_state() is called before the schedule_timeout().
The __set_current_state() is done directly in msg.c, before dropping the
lock.
--
Manfred
Re: [PATCH] ipc/msg: add msgsnd_timed and msgrcv_timed syscall for system V message queue
On Tue, 23 Feb 2021 23:11:43 +0800 Eric Gao wrote: > sometimes, we need the msgsnd or msgrcv syscall can return after a limited > time, so that the business thread do not be blocked here all the time. In > this case, I add the msgsnd_timed and msgrcv_timed syscall that with time > parameter, which has a unit of ms. Please cc Manfred and Davidlohr on ipc/ changes. The above is a very brief description for a new syscall! Please go to great lengths to tell us why this is considered useful - what are the use cases? Also, please fully describe the proposed syscall interface right here in the changelog. Please be prepared to later prepare a full manpage. > ... > +SYSCALL_DEFINE5(msgsnd_timed, int, msqid, struct msgbuf __user *, msgp, > size_t, msgsz, > + int, msgflg, long, timeoutms) Specifying the timeout in milliseconds is problematic - it's very coarse. See sys_epoll_pwait2()'s use of timespecs.
Re: [PATCH] ipc/msg: add msgsnd_timed and msgrcv_timed syscall for system V message queue
On Sun, Feb 28, 2021 at 5:16 PM Eric Gao wrote: > > > Is there something that mq_timedsend/mq_timedreceive cannot do that > > you need? Would it be possible to add that feature to the posix message > > queues instead? > > the system v message queue have a mtype parameter both in msgsnd and msgrcv > which can be > used to implement message routing(mtype as the target id. For example, I > filling the target thread > id that waiting message). It's the most important. > > but mq_timedsend/mq_timedreceive in posix message queue don't have this > feature. I'm not sure I'm following here. With posix message queues, can't you just open one queue per target thread? That would seem simpler and more efficient besides also allowing the timeout. Arnd
Re: [PATCH] ipc/msg: add msgsnd_timed and msgrcv_timed syscall for system V message queue
On Sun, Feb 28, 2021 at 4:16 PM Eric Gao wrote:
>
> Hi Arnd:
>
> Thanks for your kindly reply.
>
> I want to prove you and all of others that these syscalls are
> very useful and necessary. Actually, I add these syscalls
>
> when I try to implement the local rpc by system v message queue (some
> people might think I am crazy to using message
>
> queue, but it's truly a very efficient method than socket except it don't
> have a time-controlled msgrcv syscall).
(note: please don't reply in html)
> In addition, msgrcv_timed is also necessary in usual bidirection
> communication. For example:
> A send a message to B, and try to receive a reply from B by msgrcv syscall.
> But A need to do
> something else in case of that B has terminated. So
>
> we need the msgrcv syscall return after a preset time. The similar
> syscall can be found in
> posix message queue(mq_timedreceive) and in z/OS system of
>
> IBM(https://www.ibm.com/support/knowledgecenter/en/SSLTBW_2.2.0/com.ibm.zos.v2r2.bpxbd00/msgrcvt.htm).
>
> And when I search the web, I can find that many people need such like
> syscall in their
> applications. so I hope this patch can be merged into the mainline, Thanks a
> lot.
It might help to add some explanation on why you need to add the timeout
to the old sysv message queues, when the more modern posix message
queues already support this.
Is there something that mq_timedsend/mq_timedreceive cannot do that
you need? Would it be possible to add that feature to the posix message
queues instead?
> +COMPAT_SYSCALL_DEFINE5(msgsnd_timed, int, msqid, compat_uptr_t, msgp,
> + compat_ssize_t, msgsz, int, msgflg, compat_long_t,
> timeoutms)
> +{
> + struct compat_msgbuf __user *up = compat_ptr(msgp);
> + compat_long_t mtype;
> +
> + timeoutms = (timeoutms + 9) / 10;
> +
> + if (get_user(mtype, >mtype))
> + return -EFAULT;
> +
> + return do_msgsnd(msqid, mtype, up->mtext, (ssize_t)msgsz, msgflg,
> (long)timeoutms);
> +}
>
> My preference would be to simplify both the timed and non-timed version by
> moving the get_user() into do_msgsnd() and using in_compat_task() to pick
> the right type. Same for the receive side of course. If you do this,
> watch out for x32 support, which uses the 64-bit version.
>
> Actually, the timed and non-timed version have different number of
> parameter(timed version have timeoutms), so I don't
> think they can be combine together, and I don't want to impact the
> applications which have been using the old style msgrcv syscall.
What I meant was combining the implementation of the native and
compat versions, not combining the timed and non-timed versions,
which you already do to the degree possible.
Arnd
Re: [PATCH] ipc/msg: add msgsnd_timed and msgrcv_timed syscall for system V message queue
On Sat, Feb 27, 2021 at 7:52 AM Eric Gao wrote:
>
> sometimes, we need the msgsnd or msgrcv syscall can return after a limited
> time, so that the business thread do not be blocked here all the time. In
> this case, I add the msgsnd_timed and msgrcv_timed syscall that with time
> parameter, which has a unit of ms.
>
> Signed-off-by: Eric Gao
I have no opinion on whether we want or need this, but I'll have a look
at the implementation, to see if the ABI makes sense.
> index 8fd8c17..42b7db5 100644
> --- a/arch/mips/kernel/syscalls/syscall_n32.tbl
> +++ b/arch/mips/kernel/syscalls/syscall_n32.tbl
> @@ -381,3 +381,5 @@
> 440n32 process_madvise sys_process_madvise
> 441n32 epoll_pwait2compat_sys_epoll_pwait2
> 442n32 mount_setattr sys_mount_setattr
> +443n32 msgrcv_timedsys_msgrcv_timed
> +444n32 msgsnd_timedsys_msgsnd_timed
> diff --git a/arch/mips/kernel/syscalls/syscall_o32.tbl
> b/arch/mips/kernel/syscalls/syscall_o32.tbl
> index 090d29c..0f1f6ee 100644
> --- a/arch/mips/kernel/syscalls/syscall_o32.tbl
> +++ b/arch/mips/kernel/syscalls/syscall_o32.tbl
> @@ -430,3 +430,5 @@
> 440o32 process_madvise sys_process_madvise
> 441o32 epoll_pwait2sys_epoll_pwait2
> compat_sys_epoll_pwait2
> 442o32 mount_setattr sys_mount_setattr
> +443o32 msgrcv_timedsys_msgrcv_timed
> +444o32 msgsnd_timedsys_msgsnd_timed
I think mips n32 and o32 both need to use the compat version when running on
a 64-bit kernel, while your patch makes them use the native version.
> @@ -905,7 +906,15 @@ static long do_msgsnd(int msqid, long mtype, void __user
> *mtext,
>
> ipc_unlock_object(>q_perm);
> rcu_read_unlock();
> - schedule();
> +
> + /* sometimes, we need msgsnd syscall return after a given
> time */
> + if (timeoutms <= 0) {
> + schedule();
> + } else {
> + timeoutms = schedule_timeout(timeoutms);
> + if (timeoutms == 0)
> + timeoutflag = true;
> + }
I wonder if this should be schedule_timeout_interruptible() or at least
schedule_timeout_killable() instead of schedule_timeout(). If it should,
this should probably be done as a separate change.
> +COMPAT_SYSCALL_DEFINE5(msgsnd_timed, int, msqid, compat_uptr_t, msgp,
> + compat_ssize_t, msgsz, int, msgflg, compat_long_t,
> timeoutms)
> +{
> + struct compat_msgbuf __user *up = compat_ptr(msgp);
> + compat_long_t mtype;
> +
> + timeoutms = (timeoutms + 9) / 10;
> +
> + if (get_user(mtype, >mtype))
> + return -EFAULT;
> +
> + return do_msgsnd(msqid, mtype, up->mtext, (ssize_t)msgsz, msgflg,
> (long)timeoutms);
> +}
My preference would be to simplify both the timed and non-timed version by
moving the get_user() into do_msgsnd() and using in_compat_task() to pick
the right type. Same for the receive side of course. If you do this,
watch out for
x32 support, which uses the 64-bit version.
Arnd
