Re: [PATCH] ipc/msg: add msgsnd_timed and msgrcv_timed syscall for system V message queue

Re: [PATCH] ipc/msg: add msgsnd_timed and msgrcv_timed syscall for system V message queue

[Andrew Morton]

On Tue, 23 Feb 2021 23:11:43 +0800 Eric Gao <eric.tech@foxmail.com> 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

[Manfred Spraul]

Hi Eric,


On 3/4/21 2:12 AM, Andrew Morton wrote:
> On Tue, 23 Feb 2021 23:11:43 +0800 Eric Gao <eric.tech@foxmail.com> 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

[Arnd Bergmann]

On Sun, Feb 28, 2021 at 4:16 PM Eric Gao <eric.tech@foxmail.com> 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, &up->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

[Arnd Bergmann]

On Sun, Feb 28, 2021 at 5:16 PM Eric Gao <eric.tech@foxmail.com> 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

[Arnd Bergmann]

On Sat, Feb 27, 2021 at 7:52 AM Eric Gao <eric.tech@foxmail.com> 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 <eric.tech@foxmail.com>

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 @@
>  440    n32     process_madvise                 sys_process_madvise
>  441    n32     epoll_pwait2                    compat_sys_epoll_pwait2
>  442    n32     mount_setattr                   sys_mount_setattr
> +443    n32     msgrcv_timed                    sys_msgrcv_timed
> +444    n32     msgsnd_timed                    sys_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 @@
>  440    o32     process_madvise                 sys_process_madvise
>  441    o32     epoll_pwait2                    sys_epoll_pwait2                compat_sys_epoll_pwait2
>  442    o32     mount_setattr                   sys_mount_setattr
> +443    o32     msgrcv_timed                    sys_msgrcv_timed
> +444    o32     msgsnd_timed                    sys_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(&msq->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, &up->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