* [PATCH v2 1/2] signal: add the "int si_code" arg to prepare_kill_siginfo()
From: Oleg Nesterov @ 2024-02-09 13:06 UTC (permalink / raw)
To: Christian Brauner
Cc: Andy Lutomirski, Eric W. Biederman, Tycho Andersen, linux-api,
linux-kernel
So that do_tkill() can use this helper too. This also simplifies
the next patch.
TODO: perhaps we can kill prepare_kill_siginfo() and change the
callers to use SEND_SIG_NOINFO, but this needs some changes in
__send_signal_locked() and TP_STORE_SIGINFO().
Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
kernel/signal.c | 15 +++++----------
1 file changed, 5 insertions(+), 10 deletions(-)
diff --git a/kernel/signal.c b/kernel/signal.c
index c3fac06937e2..a8199fda0d61 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -3793,12 +3793,12 @@ COMPAT_SYSCALL_DEFINE4(rt_sigtimedwait_time32, compat_sigset_t __user *, uthese,
#endif
#endif
-static inline void prepare_kill_siginfo(int sig, struct kernel_siginfo *info)
+static void prepare_kill_siginfo(int sig, struct kernel_siginfo *info, int si_code)
{
clear_siginfo(info);
info->si_signo = sig;
info->si_errno = 0;
- info->si_code = SI_USER;
+ info->si_code = si_code;
info->si_pid = task_tgid_vnr(current);
info->si_uid = from_kuid_munged(current_user_ns(), current_uid());
}
@@ -3812,7 +3812,7 @@ SYSCALL_DEFINE2(kill, pid_t, pid, int, sig)
{
struct kernel_siginfo info;
- prepare_kill_siginfo(sig, &info);
+ prepare_kill_siginfo(sig, &info, SI_USER);
return kill_something_info(sig, &info, pid);
}
@@ -3925,7 +3925,7 @@ SYSCALL_DEFINE4(pidfd_send_signal, int, pidfd, int, sig,
(kinfo.si_code >= 0 || kinfo.si_code == SI_TKILL))
goto err;
} else {
- prepare_kill_siginfo(sig, &kinfo);
+ prepare_kill_siginfo(sig, &kinfo, SI_USER);
}
/* TODO: respect PIDFD_THREAD */
@@ -3970,12 +3970,7 @@ static int do_tkill(pid_t tgid, pid_t pid, int sig)
{
struct kernel_siginfo info;
- clear_siginfo(&info);
- info.si_signo = sig;
- info.si_errno = 0;
- info.si_code = SI_TKILL;
- info.si_pid = task_tgid_vnr(current);
- info.si_uid = from_kuid_munged(current_user_ns(), current_uid());
+ prepare_kill_siginfo(sig, &info, SI_TKILL);
return do_send_specific(tgid, pid, sig, &info);
}
--
2.25.1.362.g51ebf55
^ permalink raw reply related
* Re: [PATCH v4] ELF: AT_PAGE_SHIFT_MASK -- supply userspace with available page shifts
From: Alexey Dobriyan @ 2024-02-09 12:30 UTC (permalink / raw)
To: Kees Cook
Cc: Andrew Morton, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
Dave Hansen, Florian Weimer, linux-kernel, linux-arch, linux-api,
x86, Eric Biederman, linux-mm
In-Reply-To: <202402050445.0331B94A73@keescook>
On Mon, Feb 05, 2024 at 04:48:08AM -0800, Kees Cook wrote:
> On Mon, Feb 05, 2024 at 12:51:43PM +0300, Alexey Dobriyan wrote:
> > +#define ARCH_AT_PAGE_SHIFT_MASK \
> > + do { \
> > + u32 val = 1 << 12; \
> > + if (boot_cpu_has(X86_FEATURE_PSE)) { \
> > + val |= 1 << 21; \
> > + } \
> > + if (boot_cpu_has(X86_FEATURE_GBPAGES)) { \
> > + val |= 1 << 30; \
> > + } \
>
> Can we use something besides literal "12", "21", and "30" values here?
Ehh, no, why? Inside x86_64 the page shifts are very specific numbers,
they won't change.
^ permalink raw reply
* Re: [PATCH] pidfd: change pidfd_send_signal() to respect PIDFD_THREAD
From: Christian Brauner @ 2024-02-09 11:29 UTC (permalink / raw)
To: Oleg Nesterov
Cc: Andy Lutomirski, Eric W. Biederman, Tycho Andersen, linux-api,
linux-kernel
In-Reply-To: <20240209102816.GA3282@redhat.com>
On Fri, Feb 09, 2024 at 11:28:17AM +0100, Oleg Nesterov wrote:
> On 02/08, Oleg Nesterov wrote:
> >
> > Is prepare_kill_siginfo() correct when we send a signal to the child
> > pid namespace? si_pid = task_tgid_vnr(current) doesn't look right
>
> Yes, but iiuc send_signal_locked() should fixup si_pid/si_uid, so it
> is not buggy.
It must've been. Yesterday I realized that otherwise kill(2) would have
been broken for a long time. I think this was originally fixed in commit
6588c1e3ff01 ("signals: SI_USER: Masquerade si_pid when crossing pid ns
boundary").
^ permalink raw reply
* Re: [PATCH] pidfd: change pidfd_send_signal() to respect PIDFD_THREAD
From: Oleg Nesterov @ 2024-02-09 10:53 UTC (permalink / raw)
To: Christian Brauner
Cc: Eric W. Biederman, Andy Lutomirski, Tycho Andersen, linux-api,
linux-kernel
In-Reply-To: <20240209-postfach-notorisch-f8443677b490@brauner>
On 02/09, Christian Brauner wrote:
>
> On Thu, Feb 08, 2024 at 04:57:31PM +0100, Oleg Nesterov wrote:
> > On 02/08, Eric W. Biederman wrote:
> > >
> > > Because honestly right now using group_send_sig_info when
> > > the intended target of the signal is not the entire thread
> > > group is very confusing when reading your change.
> >
> > Agreed, so perhaps it makes sense to rename it later. See
>
> Agreed. The function seems misnamed and incorrectly documented. It just
> seems that it's never been used with PIDTYPE_PID but it's perfectly
> capable of doing that. So maybe just put a patch on top renaming it to
> send_sig_info_type() and remove the old comment. But I can live without
> renaming it for now as well.
OK, I'll update the comment, please see below.
It should probably say more about the case when type > PIDTYPE_TGID,
but this is "offtopic" for this patch.
Oleg.
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -1437,7 +1437,8 @@ void lockdep_assert_task_sighand_held(struct task_struct *task)
#endif
/*
- * send signal info to all the members of a group
+ * send signal info to all the members of a thread group or to the
+ * individual thread if type == PIDTYPE_PID.
*/
int group_send_sig_info(int sig, struct kernel_siginfo *info,
struct task_struct *p, enum pid_type type)
^ permalink raw reply
* Re: [PATCH] pidfd: change pidfd_send_signal() to respect PIDFD_THREAD
From: Oleg Nesterov @ 2024-02-09 10:28 UTC (permalink / raw)
To: Christian Brauner
Cc: Andy Lutomirski, Eric W. Biederman, Tycho Andersen, linux-api,
linux-kernel
In-Reply-To: <20240208143407.GF19801@redhat.com>
On 02/08, Oleg Nesterov wrote:
>
> Is prepare_kill_siginfo() correct when we send a signal to the child
> pid namespace? si_pid = task_tgid_vnr(current) doesn't look right
Yes, but iiuc send_signal_locked() should fixup si_pid/si_uid, so it
is not buggy.
> And why do we need it at all? Can't sys_kill() and pidfd_send_signal()
> just use SEND_SIG_NOINFO?
Probably yes. And even do_tkill() can use SEND_SIG_NOINFO if we change
__send_signal_locked() to check the type before ".si_code = SI_USER".
but then TP_STORE_SIGINFO() needs some changes...
I'll try to do this later, I do not want to mix this change with the
PIDFD_THREAD changes.
Oleg.
^ permalink raw reply
* Re: [PATCH] pidfd: change pidfd_send_signal() to respect PIDFD_THREAD
From: Christian Brauner @ 2024-02-09 9:26 UTC (permalink / raw)
To: Oleg Nesterov
Cc: Eric W. Biederman, Andy Lutomirski, Tycho Andersen, linux-api,
linux-kernel
In-Reply-To: <20240208155731.GH19801@redhat.com>
On Thu, Feb 08, 2024 at 04:57:31PM +0100, Oleg Nesterov wrote:
> On 02/08, Eric W. Biederman wrote:
> >
> > Oleg Nesterov <oleg@redhat.com> writes:
> >
> > > Turn kill_pid_info() into kill_pid_info_type(), this allows to pass any
> > > pid_type to group_send_sig_info(), despite its name it should work fine
> > > even if type = PIDTYPE_PID.
> > >
> > > Change pidfd_send_signal() to use PIDTYPE_PID or PIDTYPE_TGID depending
> > > on PIDFD_THREAD.
> > >
> > > While at it kill another TODO comment in pidfd_show_fdinfo(). As Christian
> > > expains fdinfo reports f_flags, userspace can already detect PIDFD_THREAD.
> > >
> >
> > I have a question here.
> >
> > Why is this based on group_send_sig_info instead of send_sig_info?
>
> Well. send_sig_info() accepts "struct task_struct *", not "struct pid *",
> it doesn't do check_kill_permission(), and it doesn't handle the possible
> race with mt-exec.
>
> > In particular I am asking are the intended semantics that the signal is
> > sent to a single thread in a thread group and placed in the per thread
> > queue, or is the signal sent to the entire thread group and placed
> > in the thread group signal queue?
>
> This depends on PIDFD_THREAD. If it is set then the signal goes to
> the per thread queue.
>
> > Because honestly right now using group_send_sig_info when
> > the intended target of the signal is not the entire thread
> > group is very confusing when reading your change.
>
> Agreed, so perhaps it makes sense to rename it later. See
Agreed. The function seems misnamed and incorrectly documented. It just
seems that it's never been used with PIDTYPE_PID but it's perfectly
capable of doing that. So maybe just put a patch on top renaming it to
send_sig_info_type() and remove the old comment. But I can live without
renaming it for now as well.
^ permalink raw reply
* Re: [PATCH net-next v6 1/4] eventpoll: support busy poll per epoll instance
From: Joe Damato @ 2024-02-08 18:06 UTC (permalink / raw)
To: Eric Dumazet
Cc: linux-kernel, netdev, chuck.lever, jlayton, linux-api, brauner,
davem, alexander.duyck, sridhar.samudrala, kuba,
willemdebruijn.kernel, weiwan, David.Laight, arnd, sdf,
amritha.nambiar, Alexander Viro, Jan Kara,
open list:FILESYSTEMS (VFS and infrastructure)
In-Reply-To: <CANn89iJY8mTn3PViBvTh_DewUKWjc0z3cvJvr8AcQgcbWC4G0Q@mail.gmail.com>
On Thu, Feb 08, 2024 at 06:46:25PM +0100, Eric Dumazet wrote:
> On Mon, Feb 5, 2024 at 10:05 PM Joe Damato <jdamato@fastly.com> wrote:
> >
> > Allow busy polling on a per-epoll context basis. The per-epoll context
> > usec timeout value is preferred, but the pre-existing system wide sysctl
> > value is still supported if it specified.
> >
> > Signed-off-by: Joe Damato <jdamato@fastly.com>
> > ---
> > fs/eventpoll.c | 49 +++++++++++++++++++++++++++++++++++++++++++++----
> > 1 file changed, 45 insertions(+), 4 deletions(-)
> >
> > diff --git a/fs/eventpoll.c b/fs/eventpoll.c
> > index 3534d36a1474..ce75189d46df 100644
> > --- a/fs/eventpoll.c
> > +++ b/fs/eventpoll.c
> > @@ -227,6 +227,8 @@ struct eventpoll {
> > #ifdef CONFIG_NET_RX_BUSY_POLL
> > /* used to track busy poll napi_id */
> > unsigned int napi_id;
> > + /* busy poll timeout */
> > + u64 busy_poll_usecs;
> > #endif
> >
> > #ifdef CONFIG_DEBUG_LOCK_ALLOC
> > @@ -386,12 +388,44 @@ static inline int ep_events_available(struct eventpoll *ep)
> > READ_ONCE(ep->ovflist) != EP_UNACTIVE_PTR;
> > }
> >
> > +/**
> > + * busy_loop_ep_timeout - check if busy poll has timed out. The timeout value
> > + * from the epoll instance ep is preferred, but if it is not set fallback to
> > + * the system-wide global via busy_loop_timeout.
> > + *
> > + * @start_time: The start time used to compute the remaining time until timeout.
> > + * @ep: Pointer to the eventpoll context.
> > + *
> > + * Return: true if the timeout has expired, false otherwise.
> > + */
> > +static inline bool busy_loop_ep_timeout(unsigned long start_time, struct eventpoll *ep)
> > +{
> > +#ifdef CONFIG_NET_RX_BUSY_POLL
>
> It seems this local helper is only called from code compiled when
> CONFIG_NET_RX_BUSY_POLL
> is set.
>
> Not sure why you need an #ifdef here.
Thanks, you are right.
I'll move this down to be within CONFIG_NET_RX_BUSY_POLL and get rid of the
#ifdef for the v7.
Thanks for your review.
> > + unsigned long bp_usec = READ_ONCE(ep->busy_poll_usecs);
> > +
> > + if (bp_usec) {
> > + unsigned long end_time = start_time + bp_usec;
> > + unsigned long now = busy_loop_current_time();
> > +
> > + return time_after(now, end_time);
> > + } else {
> > + return busy_loop_timeout(start_time);
> > + }
> > +#endif
> > + return true;
> > +}
> > +
> > #ifdef CONFIG_NET_RX_BUSY_POLL
> > +static bool ep_busy_loop_on(struct eventpoll *ep)
> > +{
> > + return !!ep->busy_poll_usecs || net_busy_loop_on();
> > +}
> > +
> > static bool ep_busy_loop_end(void *p, unsigned long start_time)
> > {
> > struct eventpoll *ep = p;
> >
> > - return ep_events_available(ep) || busy_loop_timeout(start_time);
> > + return ep_events_available(ep) || busy_loop_ep_timeout(start_time, ep);
> > }
> >
> > /*
> > @@ -404,7 +438,7 @@ static bool ep_busy_loop(struct eventpoll *ep, int nonblock)
> > {
> > unsigned int napi_id = READ_ONCE(ep->napi_id);
> >
> > - if ((napi_id >= MIN_NAPI_ID) && net_busy_loop_on()) {
> > + if ((napi_id >= MIN_NAPI_ID) && ep_busy_loop_on(ep)) {
> > napi_busy_loop(napi_id, nonblock ? NULL : ep_busy_loop_end, ep, false,
> > BUSY_POLL_BUDGET);
> > if (ep_events_available(ep))
> > @@ -430,7 +464,8 @@ static inline void ep_set_busy_poll_napi_id(struct epitem *epi)
> > struct socket *sock;
> > struct sock *sk;
> >
> > - if (!net_busy_loop_on())
> > + ep = epi->ep;
> > + if (!ep_busy_loop_on(ep))
> > return;
> >
> > sock = sock_from_file(epi->ffd.file);
> > @@ -442,7 +477,6 @@ static inline void ep_set_busy_poll_napi_id(struct epitem *epi)
> > return;
> >
> > napi_id = READ_ONCE(sk->sk_napi_id);
> > - ep = epi->ep;
> >
> > /* Non-NAPI IDs can be rejected
> > * or
> > @@ -466,6 +500,10 @@ static inline void ep_set_busy_poll_napi_id(struct epitem *epi)
> > {
> > }
> >
> > +static inline bool ep_busy_loop_on(struct eventpoll *ep)
> > +{
> > + return false;
> > +}
> > #endif /* CONFIG_NET_RX_BUSY_POLL */
> >
> > /*
> > @@ -2058,6 +2096,9 @@ static int do_epoll_create(int flags)
> > error = PTR_ERR(file);
> > goto out_free_fd;
> > }
> > +#ifdef CONFIG_NET_RX_BUSY_POLL
> > + ep->busy_poll_usecs = 0;
> > +#endif
> > ep->file = file;
> > fd_install(fd, file);
> > return fd;
> > --
> > 2.25.1
> >
^ permalink raw reply
* Re: [PATCH net-next v6 3/4] eventpoll: Add per-epoll prefer busy poll option
From: Eric Dumazet @ 2024-02-08 17:49 UTC (permalink / raw)
To: Jakub Kicinski
Cc: Joe Damato, linux-kernel, netdev, chuck.lever, jlayton, linux-api,
brauner, davem, alexander.duyck, sridhar.samudrala,
willemdebruijn.kernel, weiwan, David.Laight, arnd, sdf,
amritha.nambiar, Alexander Viro, Jan Kara,
open list:FILESYSTEMS (VFS and infrastructure)
In-Reply-To: <20240207110437.292f7eaf@kernel.org>
On Wed, Feb 7, 2024 at 8:04 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Mon, 5 Feb 2024 21:04:48 +0000 Joe Damato wrote:
> > When using epoll-based busy poll, the prefer_busy_poll option is hardcoded
> > to false. Users may want to enable prefer_busy_poll to be used in
> > conjunction with gro_flush_timeout and defer_hard_irqs_count to keep device
> > IRQs masked.
> >
> > Other busy poll methods allow enabling or disabling prefer busy poll via
> > SO_PREFER_BUSY_POLL, but epoll-based busy polling uses a hardcoded value.
> >
> > Fix this edge case by adding support for a per-epoll context
> > prefer_busy_poll option. The default is false, as it was hardcoded before
> > this change.
>
> Reviewed-by: Jakub Kicinski <kuba@kernel.org>
Reviewed-by: Eric Dumazet <edumazet@google.com>
^ permalink raw reply
* Re: [PATCH net-next v6 2/4] eventpoll: Add per-epoll busy poll packet budget
From: Eric Dumazet @ 2024-02-08 17:47 UTC (permalink / raw)
To: Jakub Kicinski
Cc: Joe Damato, linux-kernel, netdev, chuck.lever, jlayton, linux-api,
brauner, davem, alexander.duyck, sridhar.samudrala,
willemdebruijn.kernel, weiwan, David.Laight, arnd, sdf,
amritha.nambiar, Alexander Viro, Jan Kara,
open list:FILESYSTEMS (VFS and infrastructure)
In-Reply-To: <20240207110429.7fbf391e@kernel.org>
On Wed, Feb 7, 2024 at 8:04 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Mon, 5 Feb 2024 21:04:47 +0000 Joe Damato wrote:
> > When using epoll-based busy poll, the packet budget is hardcoded to
> > BUSY_POLL_BUDGET (8). Users may desire larger busy poll budgets, which
> > can potentially increase throughput when busy polling under high network
> > load.
> >
> > Other busy poll methods allow setting the busy poll budget via
> > SO_BUSY_POLL_BUDGET, but epoll-based busy polling uses a hardcoded
> > value.
> >
> > Fix this edge case by adding support for a per-epoll context busy poll
> > packet budget. If not specified, the default value (BUSY_POLL_BUDGET) is
> > used.
>
> Reviewed-by: Jakub Kicinski <kuba@kernel.org>
Reviewed-by: Eric Dumazet <edumazet@google.com>
^ permalink raw reply
* Re: [PATCH net-next v6 1/4] eventpoll: support busy poll per epoll instance
From: Eric Dumazet @ 2024-02-08 17:46 UTC (permalink / raw)
To: Joe Damato
Cc: linux-kernel, netdev, chuck.lever, jlayton, linux-api, brauner,
davem, alexander.duyck, sridhar.samudrala, kuba,
willemdebruijn.kernel, weiwan, David.Laight, arnd, sdf,
amritha.nambiar, Alexander Viro, Jan Kara,
open list:FILESYSTEMS (VFS and infrastructure)
In-Reply-To: <20240205210453.11301-2-jdamato@fastly.com>
On Mon, Feb 5, 2024 at 10:05 PM Joe Damato <jdamato@fastly.com> wrote:
>
> Allow busy polling on a per-epoll context basis. The per-epoll context
> usec timeout value is preferred, but the pre-existing system wide sysctl
> value is still supported if it specified.
>
> Signed-off-by: Joe Damato <jdamato@fastly.com>
> ---
> fs/eventpoll.c | 49 +++++++++++++++++++++++++++++++++++++++++++++----
> 1 file changed, 45 insertions(+), 4 deletions(-)
>
> diff --git a/fs/eventpoll.c b/fs/eventpoll.c
> index 3534d36a1474..ce75189d46df 100644
> --- a/fs/eventpoll.c
> +++ b/fs/eventpoll.c
> @@ -227,6 +227,8 @@ struct eventpoll {
> #ifdef CONFIG_NET_RX_BUSY_POLL
> /* used to track busy poll napi_id */
> unsigned int napi_id;
> + /* busy poll timeout */
> + u64 busy_poll_usecs;
> #endif
>
> #ifdef CONFIG_DEBUG_LOCK_ALLOC
> @@ -386,12 +388,44 @@ static inline int ep_events_available(struct eventpoll *ep)
> READ_ONCE(ep->ovflist) != EP_UNACTIVE_PTR;
> }
>
> +/**
> + * busy_loop_ep_timeout - check if busy poll has timed out. The timeout value
> + * from the epoll instance ep is preferred, but if it is not set fallback to
> + * the system-wide global via busy_loop_timeout.
> + *
> + * @start_time: The start time used to compute the remaining time until timeout.
> + * @ep: Pointer to the eventpoll context.
> + *
> + * Return: true if the timeout has expired, false otherwise.
> + */
> +static inline bool busy_loop_ep_timeout(unsigned long start_time, struct eventpoll *ep)
> +{
> +#ifdef CONFIG_NET_RX_BUSY_POLL
It seems this local helper is only called from code compiled when
CONFIG_NET_RX_BUSY_POLL
is set.
Not sure why you need an #ifdef here.
> + unsigned long bp_usec = READ_ONCE(ep->busy_poll_usecs);
> +
> + if (bp_usec) {
> + unsigned long end_time = start_time + bp_usec;
> + unsigned long now = busy_loop_current_time();
> +
> + return time_after(now, end_time);
> + } else {
> + return busy_loop_timeout(start_time);
> + }
> +#endif
> + return true;
> +}
> +
> #ifdef CONFIG_NET_RX_BUSY_POLL
> +static bool ep_busy_loop_on(struct eventpoll *ep)
> +{
> + return !!ep->busy_poll_usecs || net_busy_loop_on();
> +}
> +
> static bool ep_busy_loop_end(void *p, unsigned long start_time)
> {
> struct eventpoll *ep = p;
>
> - return ep_events_available(ep) || busy_loop_timeout(start_time);
> + return ep_events_available(ep) || busy_loop_ep_timeout(start_time, ep);
> }
>
> /*
> @@ -404,7 +438,7 @@ static bool ep_busy_loop(struct eventpoll *ep, int nonblock)
> {
> unsigned int napi_id = READ_ONCE(ep->napi_id);
>
> - if ((napi_id >= MIN_NAPI_ID) && net_busy_loop_on()) {
> + if ((napi_id >= MIN_NAPI_ID) && ep_busy_loop_on(ep)) {
> napi_busy_loop(napi_id, nonblock ? NULL : ep_busy_loop_end, ep, false,
> BUSY_POLL_BUDGET);
> if (ep_events_available(ep))
> @@ -430,7 +464,8 @@ static inline void ep_set_busy_poll_napi_id(struct epitem *epi)
> struct socket *sock;
> struct sock *sk;
>
> - if (!net_busy_loop_on())
> + ep = epi->ep;
> + if (!ep_busy_loop_on(ep))
> return;
>
> sock = sock_from_file(epi->ffd.file);
> @@ -442,7 +477,6 @@ static inline void ep_set_busy_poll_napi_id(struct epitem *epi)
> return;
>
> napi_id = READ_ONCE(sk->sk_napi_id);
> - ep = epi->ep;
>
> /* Non-NAPI IDs can be rejected
> * or
> @@ -466,6 +500,10 @@ static inline void ep_set_busy_poll_napi_id(struct epitem *epi)
> {
> }
>
> +static inline bool ep_busy_loop_on(struct eventpoll *ep)
> +{
> + return false;
> +}
> #endif /* CONFIG_NET_RX_BUSY_POLL */
>
> /*
> @@ -2058,6 +2096,9 @@ static int do_epoll_create(int flags)
> error = PTR_ERR(file);
> goto out_free_fd;
> }
> +#ifdef CONFIG_NET_RX_BUSY_POLL
> + ep->busy_poll_usecs = 0;
> +#endif
> ep->file = file;
> fd_install(fd, file);
> return fd;
> --
> 2.25.1
>
^ permalink raw reply
* Re: [PATCH] pidfd: change pidfd_send_signal() to respect PIDFD_THREAD
From: Oleg Nesterov @ 2024-02-08 16:11 UTC (permalink / raw)
To: Christian Brauner
Cc: Andy Lutomirski, Eric W. Biederman, Tycho Andersen, linux-api,
linux-kernel
In-Reply-To: <20240208-klopapier-aushebeln-446ac80a6e9b@brauner>
On 02/08, Christian Brauner wrote:
>
> > Is prepare_kill_siginfo() correct when we send a signal to the child
> > pid namespace? si_pid = task_tgid_vnr(current) doesn't look right in
> > this case but perhaps I am totally confused.
> >
> > And why do we need it at all? Can't sys_kill() and pidfd_send_signal()
> > just use SEND_SIG_NOINFO?
>
> Yeah, good point. I don't remember as it's been quite a while ago. My
> guess is that it just tried to mirror kill() itself without being aware
> of SEND_SIG_NOINFO. If you don't find anything wrong with this then
> switch it to SEND_SIG_NOINFO in a preparatory patch we can backport,
> please.
Yes, but I still feel I must have missed something. Will read this code
tomorrow.
And another note for the record before I forget this. We can probably
improve and rename access_pidfd_pidns(). Currently it is only used by
pidfd_send_signal() but pidns_install() looks like another user.
Oleg.
^ permalink raw reply
* Re: [PATCH] pidfd: change pidfd_send_signal() to respect PIDFD_THREAD
From: Eric W. Biederman @ 2024-02-08 15:33 UTC (permalink / raw)
To: Oleg Nesterov
Cc: Christian Brauner, Andy Lutomirski, Tycho Andersen, linux-api,
linux-kernel
In-Reply-To: <20240207114549.GA12697@redhat.com>
Oleg Nesterov <oleg@redhat.com> writes:
> Turn kill_pid_info() into kill_pid_info_type(), this allows to pass any
> pid_type to group_send_sig_info(), despite its name it should work fine
> even if type = PIDTYPE_PID.
>
> Change pidfd_send_signal() to use PIDTYPE_PID or PIDTYPE_TGID depending
> on PIDFD_THREAD.
>
> While at it kill another TODO comment in pidfd_show_fdinfo(). As Christian
> expains fdinfo reports f_flags, userspace can already detect PIDFD_THREAD.
>
I have a question here.
Why is this based on group_send_sig_info instead of send_sig_info?
AKA why does this look like sys_kill instead of sys_tgkill?
In particular I am asking are the intended semantics that the signal is
sent to a single thread in a thread group and placed in the per thread
queue, or is the signal sent to the entire thread group and placed
in the thread group signal queue?
Does the type parameter handle that decision for us now? If so
perhaps we should cleanup the helpers so that this easier to
see when reading the code.
Because honestly right now using group_send_sig_info when
the intended target of the signal is not the entire thread
group is very confusing when reading your change.
Eric
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>
> ---
> kernel/fork.c | 2 --
> kernel/signal.c | 18 ++++++++++++------
> 2 files changed, 12 insertions(+), 8 deletions(-)
>
> diff --git a/kernel/fork.c b/kernel/fork.c
> index cd61ca87d0e6..47b565598063 100644
> --- a/kernel/fork.c
> +++ b/kernel/fork.c
> @@ -2051,8 +2051,6 @@ static void pidfd_show_fdinfo(struct seq_file *m, struct file *f)
>
> seq_put_decimal_ll(m, "Pid:\t", nr);
>
> - /* TODO: report PIDFD_THREAD */
> -
> #ifdef CONFIG_PID_NS
> seq_put_decimal_ll(m, "\nNSpid:\t", nr);
> if (nr > 0) {
> diff --git a/kernel/signal.c b/kernel/signal.c
> index c3fac06937e2..e3edcd784e45 100644
> --- a/kernel/signal.c
> +++ b/kernel/signal.c
> @@ -47,6 +47,7 @@
> #include <linux/cgroup.h>
> #include <linux/audit.h>
> #include <linux/sysctl.h>
> +#include <uapi/linux/pidfd.h>
>
> #define CREATE_TRACE_POINTS
> #include <trace/events/signal.h>
> @@ -1478,7 +1479,8 @@ int __kill_pgrp_info(int sig, struct kernel_siginfo *info, struct pid *pgrp)
> return ret;
> }
>
> -int kill_pid_info(int sig, struct kernel_siginfo *info, struct pid *pid)
> +static int kill_pid_info_type(int sig, struct kernel_siginfo *info,
> + struct pid *pid, enum pid_type type)
> {
> int error = -ESRCH;
> struct task_struct *p;
> @@ -1487,11 +1489,10 @@ int kill_pid_info(int sig, struct kernel_siginfo *info, struct pid *pid)
> rcu_read_lock();
> p = pid_task(pid, PIDTYPE_PID);
> if (p)
> - error = group_send_sig_info(sig, info, p, PIDTYPE_TGID);
> + error = group_send_sig_info(sig, info, p, type);
> rcu_read_unlock();
> if (likely(!p || error != -ESRCH))
> return error;
> -
> /*
> * The task was unhashed in between, try again. If it
> * is dead, pid_task() will return NULL, if we race with
> @@ -1500,6 +1501,11 @@ int kill_pid_info(int sig, struct kernel_siginfo *info, struct pid *pid)
> }
> }
>
> +int kill_pid_info(int sig, struct kernel_siginfo *info, struct pid *pid)
> +{
> + return kill_pid_info_type(sig, info, pid, PIDTYPE_TGID);
> +}
> +
> static int kill_proc_info(int sig, struct kernel_siginfo *info, pid_t pid)
> {
> int error;
> @@ -3890,6 +3896,7 @@ SYSCALL_DEFINE4(pidfd_send_signal, int, pidfd, int, sig,
> struct fd f;
> struct pid *pid;
> kernel_siginfo_t kinfo;
> + enum pid_type type;
>
> /* Enforce flags be set to 0 until we add an extension. */
> if (flags)
> @@ -3928,9 +3935,8 @@ SYSCALL_DEFINE4(pidfd_send_signal, int, pidfd, int, sig,
> prepare_kill_siginfo(sig, &kinfo);
> }
>
> - /* TODO: respect PIDFD_THREAD */
> - ret = kill_pid_info(sig, &kinfo, pid);
> -
> + type = (f.file->f_flags & PIDFD_THREAD) ? PIDTYPE_PID : PIDTYPE_TGID;
> + ret = kill_pid_info_type(sig, &kinfo, pid, type);
> err:
> fdput(f);
> return ret;
^ permalink raw reply
* Re: [PATCH] pidfd: change pidfd_send_signal() to respect PIDFD_THREAD
From: Oleg Nesterov @ 2024-02-08 15:57 UTC (permalink / raw)
To: Eric W. Biederman
Cc: Christian Brauner, Andy Lutomirski, Tycho Andersen, linux-api,
linux-kernel
In-Reply-To: <8734u32co5.fsf@email.froward.int.ebiederm.org>
On 02/08, Eric W. Biederman wrote:
>
> Oleg Nesterov <oleg@redhat.com> writes:
>
> > Turn kill_pid_info() into kill_pid_info_type(), this allows to pass any
> > pid_type to group_send_sig_info(), despite its name it should work fine
> > even if type = PIDTYPE_PID.
> >
> > Change pidfd_send_signal() to use PIDTYPE_PID or PIDTYPE_TGID depending
> > on PIDFD_THREAD.
> >
> > While at it kill another TODO comment in pidfd_show_fdinfo(). As Christian
> > expains fdinfo reports f_flags, userspace can already detect PIDFD_THREAD.
> >
>
> I have a question here.
>
> Why is this based on group_send_sig_info instead of send_sig_info?
Well. send_sig_info() accepts "struct task_struct *", not "struct pid *",
it doesn't do check_kill_permission(), and it doesn't handle the possible
race with mt-exec.
> In particular I am asking are the intended semantics that the signal is
> sent to a single thread in a thread group and placed in the per thread
> queue, or is the signal sent to the entire thread group and placed
> in the thread group signal queue?
This depends on PIDFD_THREAD. If it is set then the signal goes to
the per thread queue.
> Because honestly right now using group_send_sig_info when
> the intended target of the signal is not the entire thread
> group is very confusing when reading your change.
Agreed, so perhaps it makes sense to rename it later. See
despite its name it should work fine even if type = PIDTYPE_PID.
in the changelog above.
Oleg.
^ permalink raw reply
* Re: [PATCH] pidfd: change pidfd_send_signal() to respect PIDFD_THREAD
From: Christian Brauner @ 2024-02-08 15:33 UTC (permalink / raw)
To: Oleg Nesterov
Cc: Andy Lutomirski, Eric W. Biederman, Tycho Andersen, linux-api,
linux-kernel
In-Reply-To: <20240208143407.GF19801@redhat.com>
> Is prepare_kill_siginfo() correct when we send a signal to the child
> pid namespace? si_pid = task_tgid_vnr(current) doesn't look right in
> this case but perhaps I am totally confused.
>
> And why do we need it at all? Can't sys_kill() and pidfd_send_signal()
> just use SEND_SIG_NOINFO?
Yeah, good point. I don't remember as it's been quite a while ago. My
guess is that it just tried to mirror kill() itself without being aware
of SEND_SIG_NOINFO. If you don't find anything wrong with this then
switch it to SEND_SIG_NOINFO in a preparatory patch we can backport,
please.
^ permalink raw reply
* Re: [PATCH] pidfd: change pidfd_send_signal() to respect PIDFD_THREAD
From: Oleg Nesterov @ 2024-02-08 14:34 UTC (permalink / raw)
To: Christian Brauner
Cc: Andy Lutomirski, Eric W. Biederman, Tycho Andersen, linux-api,
linux-kernel
In-Reply-To: <20240208135344.GD19801@redhat.com>
On 02/08, Oleg Nesterov wrote:
>
> On 02/08, Christian Brauner wrote:
> >
> > On Wed, Feb 07, 2024 at 12:45:49PM +0100, Oleg Nesterov wrote:
> > > + type = (f.file->f_flags & PIDFD_THREAD) ? PIDTYPE_PID : PIDTYPE_TGID;
> > > + ret = kill_pid_info_type(sig, &kinfo, pid, type);
> >
> > If the user doesn't provide siginfo then the kernel fills in the info in
> > prepare_kill_siginfo() a few lines above. That sets info->si_code to
> > SI_USER even for the PIDFD_THREAD case. Whenever the info is filled in
> > by the kernel it's not exactly userspace impersonating anything plus we
> > know that what we're sending to is a pidfd by the type of the pidfd. So
> > it feels like we should fill in SI_TKILL here as well?
>
> Hmm. Agreed, will do, thanks.
Cough... lets forget this patch for the moment.
Is prepare_kill_siginfo() correct when we send a signal to the child
pid namespace? si_pid = task_tgid_vnr(current) doesn't look right in
this case but perhaps I am totally confused.
And why do we need it at all? Can't sys_kill() and pidfd_send_signal()
just use SEND_SIG_NOINFO?
OK, I am sure I missed something. Will read this code tomorrow.
Oleg.
^ permalink raw reply
* Re: [PATCH] pidfd: change pidfd_send_signal() to respect PIDFD_THREAD
From: Christian Brauner @ 2024-02-08 14:33 UTC (permalink / raw)
To: Oleg Nesterov
Cc: Andy Lutomirski, Eric W. Biederman, Tycho Andersen, linux-api,
linux-kernel
In-Reply-To: <20240208140610.GE19801@redhat.com>
On Thu, Feb 08, 2024 at 03:06:10PM +0100, Oleg Nesterov wrote:
> On 02/08, Christian Brauner wrote:
> >
> > I would also suggest we update the obsolete comment on top of
> > pidfd_send_signal() along the lines of:
>
> Yes, but...
>
> > + * If the @pidfd refers to a thread-group leader the signal is thread-group
> > + * directed. If @pidfd referes to a thread then the signal is thread directed.
>
> No, this depends on PIDFD_THREAD only.
Sorry, yes, that is what I meant to say but obviously wrote unclearly. I
mean that if pidfd->f_flags & PIDFD_THREAD then it's thread-directed.
That's what I meant by type of pidfd. Not type as in PIDTYPE_PID in
struct pid.
>
> If it is set then the signal is always "thread directed" even if @pidfd refers
> to a thread-group leader.
>
> Otherwise the target task must be a group leader and the signal will be
> "thread-group directed".
>
> Right?
Yes, please feel free to update the comment!
^ permalink raw reply
* Re: [PATCH] pidfd: change pidfd_send_signal() to respect PIDFD_THREAD
From: Christian Brauner @ 2024-02-08 14:31 UTC (permalink / raw)
To: Oleg Nesterov
Cc: Andy Lutomirski, Eric W. Biederman, Tycho Andersen, linux-api,
linux-kernel
In-Reply-To: <20240208135344.GD19801@redhat.com>
On Thu, Feb 08, 2024 at 02:53:45PM +0100, Oleg Nesterov wrote:
> On 02/08, Christian Brauner wrote:
> >
> > On Wed, Feb 07, 2024 at 12:45:49PM +0100, Oleg Nesterov wrote:
> > > + type = (f.file->f_flags & PIDFD_THREAD) ? PIDTYPE_PID : PIDTYPE_TGID;
> > > + ret = kill_pid_info_type(sig, &kinfo, pid, type);
> >
> > If the user doesn't provide siginfo then the kernel fills in the info in
> > prepare_kill_siginfo() a few lines above. That sets info->si_code to
> > SI_USER even for the PIDFD_THREAD case. Whenever the info is filled in
> > by the kernel it's not exactly userspace impersonating anything plus we
> > know that what we're sending to is a pidfd by the type of the pidfd. So
> > it feels like we should fill in SI_TKILL here as well?
>
> Hmm. Agreed, will do, thanks.
>
> But then I think this needs another preparational 1/2 patch.
> prepare_kill_siginfo() should have a new arg so that do_tkill() could
> use it too.
Agreed.
>
> (offtopic, but may be the "Only allow sending arbitrary signals to yourself"
> check in pidfd_send_signal() needs another helper, do_rt_sigqueueinfo()
> does the same check).
Agreed.
^ permalink raw reply
* Re: [PATCH] pidfd: change pidfd_send_signal() to respect PIDFD_THREAD
From: Oleg Nesterov @ 2024-02-08 14:06 UTC (permalink / raw)
To: Christian Brauner
Cc: Andy Lutomirski, Eric W. Biederman, Tycho Andersen, linux-api,
linux-kernel
In-Reply-To: <20240208-fragt-prospekt-7866333b15f0@brauner>
On 02/08, Christian Brauner wrote:
>
> I would also suggest we update the obsolete comment on top of
> pidfd_send_signal() along the lines of:
Yes, but...
> + * If the @pidfd refers to a thread-group leader the signal is thread-group
> + * directed. If @pidfd referes to a thread then the signal is thread directed.
No, this depends on PIDFD_THREAD only.
If it is set then the signal is always "thread directed" even if @pidfd refers
to a thread-group leader.
Otherwise the target task must be a group leader and the signal will be
"thread-group directed".
Right?
Oleg.
^ permalink raw reply
* Re: [PATCH] pidfd: change pidfd_send_signal() to respect PIDFD_THREAD
From: Oleg Nesterov @ 2024-02-08 13:53 UTC (permalink / raw)
To: Christian Brauner
Cc: Andy Lutomirski, Eric W. Biederman, Tycho Andersen, linux-api,
linux-kernel
In-Reply-To: <20240208-fragt-prospekt-7866333b15f0@brauner>
On 02/08, Christian Brauner wrote:
>
> On Wed, Feb 07, 2024 at 12:45:49PM +0100, Oleg Nesterov wrote:
> > + type = (f.file->f_flags & PIDFD_THREAD) ? PIDTYPE_PID : PIDTYPE_TGID;
> > + ret = kill_pid_info_type(sig, &kinfo, pid, type);
>
> If the user doesn't provide siginfo then the kernel fills in the info in
> prepare_kill_siginfo() a few lines above. That sets info->si_code to
> SI_USER even for the PIDFD_THREAD case. Whenever the info is filled in
> by the kernel it's not exactly userspace impersonating anything plus we
> know that what we're sending to is a pidfd by the type of the pidfd. So
> it feels like we should fill in SI_TKILL here as well?
Hmm. Agreed, will do, thanks.
But then I think this needs another preparational 1/2 patch.
prepare_kill_siginfo() should have a new arg so that do_tkill() could
use it too.
(offtopic, but may be the "Only allow sending arbitrary signals to yourself"
check in pidfd_send_signal() needs another helper, do_rt_sigqueueinfo()
does the same check).
> I would also suggest we update the obsolete comment on top of
> pidfd_send_signal() along the lines of:
Ah, indeed, thanks.
Oleg.
^ permalink raw reply
* Re: [PATCH] pidfd: change pidfd_send_signal() to respect PIDFD_THREAD
From: Christian Brauner @ 2024-02-08 13:15 UTC (permalink / raw)
To: Oleg Nesterov
Cc: Andy Lutomirski, Eric W. Biederman, Tycho Andersen, linux-api,
linux-kernel
In-Reply-To: <20240207114549.GA12697@redhat.com>
On Wed, Feb 07, 2024 at 12:45:49PM +0100, Oleg Nesterov wrote:
> Turn kill_pid_info() into kill_pid_info_type(), this allows to pass any
> pid_type to group_send_sig_info(), despite its name it should work fine
> even if type = PIDTYPE_PID.
>
> Change pidfd_send_signal() to use PIDTYPE_PID or PIDTYPE_TGID depending
> on PIDFD_THREAD.
>
> While at it kill another TODO comment in pidfd_show_fdinfo(). As Christian
> expains fdinfo reports f_flags, userspace can already detect PIDFD_THREAD.
>
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>
> ---
> kernel/fork.c | 2 --
> kernel/signal.c | 18 ++++++++++++------
> 2 files changed, 12 insertions(+), 8 deletions(-)
>
> diff --git a/kernel/fork.c b/kernel/fork.c
> index cd61ca87d0e6..47b565598063 100644
> --- a/kernel/fork.c
> +++ b/kernel/fork.c
> @@ -2051,8 +2051,6 @@ static void pidfd_show_fdinfo(struct seq_file *m, struct file *f)
>
> seq_put_decimal_ll(m, "Pid:\t", nr);
>
> - /* TODO: report PIDFD_THREAD */
> -
> #ifdef CONFIG_PID_NS
> seq_put_decimal_ll(m, "\nNSpid:\t", nr);
> if (nr > 0) {
> diff --git a/kernel/signal.c b/kernel/signal.c
> index c3fac06937e2..e3edcd784e45 100644
> --- a/kernel/signal.c
> +++ b/kernel/signal.c
> @@ -47,6 +47,7 @@
> #include <linux/cgroup.h>
> #include <linux/audit.h>
> #include <linux/sysctl.h>
> +#include <uapi/linux/pidfd.h>
>
> #define CREATE_TRACE_POINTS
> #include <trace/events/signal.h>
> @@ -1478,7 +1479,8 @@ int __kill_pgrp_info(int sig, struct kernel_siginfo *info, struct pid *pgrp)
> return ret;
> }
>
> -int kill_pid_info(int sig, struct kernel_siginfo *info, struct pid *pid)
> +static int kill_pid_info_type(int sig, struct kernel_siginfo *info,
> + struct pid *pid, enum pid_type type)
> {
> int error = -ESRCH;
> struct task_struct *p;
> @@ -1487,11 +1489,10 @@ int kill_pid_info(int sig, struct kernel_siginfo *info, struct pid *pid)
> rcu_read_lock();
> p = pid_task(pid, PIDTYPE_PID);
> if (p)
> - error = group_send_sig_info(sig, info, p, PIDTYPE_TGID);
> + error = group_send_sig_info(sig, info, p, type);
> rcu_read_unlock();
> if (likely(!p || error != -ESRCH))
> return error;
> -
> /*
> * The task was unhashed in between, try again. If it
> * is dead, pid_task() will return NULL, if we race with
> @@ -1500,6 +1501,11 @@ int kill_pid_info(int sig, struct kernel_siginfo *info, struct pid *pid)
> }
> }
>
> +int kill_pid_info(int sig, struct kernel_siginfo *info, struct pid *pid)
> +{
> + return kill_pid_info_type(sig, info, pid, PIDTYPE_TGID);
> +}
> +
> static int kill_proc_info(int sig, struct kernel_siginfo *info, pid_t pid)
> {
> int error;
> @@ -3890,6 +3896,7 @@ SYSCALL_DEFINE4(pidfd_send_signal, int, pidfd, int, sig,
> struct fd f;
> struct pid *pid;
> kernel_siginfo_t kinfo;
> + enum pid_type type;
>
> /* Enforce flags be set to 0 until we add an extension. */
> if (flags)
> @@ -3928,9 +3935,8 @@ SYSCALL_DEFINE4(pidfd_send_signal, int, pidfd, int, sig,
> prepare_kill_siginfo(sig, &kinfo);
> }
>
> - /* TODO: respect PIDFD_THREAD */
> - ret = kill_pid_info(sig, &kinfo, pid);
> -
> + type = (f.file->f_flags & PIDFD_THREAD) ? PIDTYPE_PID : PIDTYPE_TGID;
> + ret = kill_pid_info_type(sig, &kinfo, pid, type);
If the user doesn't provide siginfo then the kernel fills in the info in
prepare_kill_siginfo() a few lines above. That sets info->si_code to
SI_USER even for the PIDFD_THREAD case. Whenever the info is filled in
by the kernel it's not exactly userspace impersonating anything plus we
know that what we're sending to is a pidfd by the type of the pidfd. So
it feels like we should fill in SI_TKILL here as well?
I would also suggest we update the obsolete comment on top of
pidfd_send_signal() along the lines of:
diff --git a/kernel/signal.c b/kernel/signal.c
index e3edcd784e45..40df0c17abd7 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -3878,14 +3878,10 @@ static struct pid *pidfd_to_pid(const struct file *file)
* @info: signal info
* @flags: future flags
*
- * The syscall currently only signals via PIDTYPE_PID which covers
- * kill(<positive-pid>, <signal>. It does not signal threads or process
- * groups.
- * In order to extend the syscall to threads and process groups the @flags
- * argument should be used. In essence, the @flags argument will determine
- * what is signaled and not the file descriptor itself. Put in other words,
- * grouping is a property of the flags argument not a property of the file
- * descriptor.
+ * If the @pidfd refers to a thread-group leader the signal is thread-group
+ * directed. If @pidfd referes to a thread then the signal is thread directed.
+ * In the future extension to @flags may be used to override the default scope
+ * of @pidfd.
*
* Return: 0 on success, negative errno on failure
*/
^ permalink raw reply related
* Re: [PATCH net-next v6 1/4] eventpoll: support busy poll per epoll instance
From: Jakub Kicinski @ 2024-02-07 20:56 UTC (permalink / raw)
To: Joe Damato
Cc: linux-kernel, netdev, chuck.lever, jlayton, linux-api, brauner,
edumazet, davem, alexander.duyck, sridhar.samudrala,
willemdebruijn.kernel, weiwan, David.Laight, arnd, sdf,
amritha.nambiar, Alexander Viro, Jan Kara,
open list:FILESYSTEMS (VFS and infrastructure)
In-Reply-To: <20240207202323.GA1283@fastly.com>
On Wed, 7 Feb 2024 12:23:23 -0800 Joe Damato wrote:
> > Unless you have a clear reason not to, I think using u32 would be more
> > natural? If my head math is right the range for u32 is 4096 sec,
> > slightly over an hour? I'd use u32 and limit it to S32_MAX.
>
> OK, that seems fine. Sorry for the noob question, but since that represents
> a fucntional change to patch 4/4, I believe I would need to drop Jiri's
> Reviewed-by, is that right?
I'd default to keeping it. But the review tag retention rules are one
of the more subjective things in kernel developments.
^ permalink raw reply
* Re: [PATCH net-next v6 1/4] eventpoll: support busy poll per epoll instance
From: Joe Damato @ 2024-02-07 20:23 UTC (permalink / raw)
To: Jakub Kicinski
Cc: linux-kernel, netdev, chuck.lever, jlayton, linux-api, brauner,
edumazet, davem, alexander.duyck, sridhar.samudrala,
willemdebruijn.kernel, weiwan, David.Laight, arnd, sdf,
amritha.nambiar, Alexander Viro, Jan Kara,
open list:FILESYSTEMS (VFS and infrastructure)
In-Reply-To: <20240207121124.12941ed9@kernel.org>
On Wed, Feb 07, 2024 at 12:11:24PM -0800, Jakub Kicinski wrote:
> On Wed, 7 Feb 2024 11:14:08 -0800 Joe Damato wrote:
> > > Why do we need u64 for usecs? I think u16 would do, and u32 would give
> > > a very solid "engineering margin". If it was discussed in previous
> > > versions I think it's worth explaining in the commit message.
> >
> > In patch 4/4 the value is limited to U32_MAX, but if you prefer I use a u32
> > here instead, I can make that change.
>
> Unless you have a clear reason not to, I think using u32 would be more
> natural? If my head math is right the range for u32 is 4096 sec,
> slightly over an hour? I'd use u32 and limit it to S32_MAX.
OK, that seems fine. Sorry for the noob question, but since that represents
a fucntional change to patch 4/4, I believe I would need to drop Jiri's
Reviewed-by, is that right?
^ permalink raw reply
* Re: [PATCH net-next v6 4/4] eventpoll: Add epoll ioctl for epoll_params
From: Jakub Kicinski @ 2024-02-07 20:18 UTC (permalink / raw)
To: Joe Damato
Cc: netdev, linux-kernel, chuck.lever, jlayton, linux-api, brauner,
edumazet, davem, alexander.duyck, sridhar.samudrala,
willemdebruijn.kernel, weiwan, David.Laight, arnd, sdf,
amritha.nambiar, Jonathan Corbet, Alexander Viro, Jan Kara,
Nathan Lynch, Michael Ellerman, Greg Kroah-Hartman, Namjae Jeon,
Steve French, Thomas Zimmermann, Julien Panis, Andrew Waterman,
Palmer Dabbelt, Albert Ou, open list:DOCUMENTATION,
open list:FILESYSTEMS (VFS and infrastructure)
In-Reply-To: <20240207191603.GB1313@fastly.com>
On Wed, 7 Feb 2024 11:16:03 -0800 Joe Damato wrote:
> > > netdev maintainers: Jiri marked this with Reviewed-by, but was this review
> > > what caused "Changes Requested" to be the status set for this patch set in
> > > patchwork?
> > >
> > > If needed, I'll send a v7 with the changes Jiri suggested and add the
> > > "Reviewed-by" since the changes are cosmetic, but I wanted to make sure
> > > this was the reason.
> >
> > Yes, I think that's it.
>
> OK, thanks for letting me know. I wasn't sure if it was because of the
> netdev/source_inline which marked 1/4 as "fail" because of the inlines
> added.
>
> Does that need to be changed, as well?
For background our preference is to avoid using static inline in C
sources, unless the author compiled the code and actually confirmed
the code doesn't get inlined correctly. But it's not a hard
requirement, and technically the code is under fs/.
In general the patchwork checks are a bit noisy, see here the top left
graph of how many of the patches we merge are "all green":
https://netdev.bots.linux.dev/checks.html
Some of the checks are also largely outside of our control (checkpatch)
so consider the patchwork checks as automation for maintainers.
The maintainers should respond on the list if any of the failures
are indeed legit.
^ permalink raw reply
* Re: [PATCH net-next v6 1/4] eventpoll: support busy poll per epoll instance
From: Jakub Kicinski @ 2024-02-07 20:11 UTC (permalink / raw)
To: Joe Damato
Cc: linux-kernel, netdev, chuck.lever, jlayton, linux-api, brauner,
edumazet, davem, alexander.duyck, sridhar.samudrala,
willemdebruijn.kernel, weiwan, David.Laight, arnd, sdf,
amritha.nambiar, Alexander Viro, Jan Kara,
open list:FILESYSTEMS (VFS and infrastructure)
In-Reply-To: <20240207191407.GA1313@fastly.com>
On Wed, 7 Feb 2024 11:14:08 -0800 Joe Damato wrote:
> > Why do we need u64 for usecs? I think u16 would do, and u32 would give
> > a very solid "engineering margin". If it was discussed in previous
> > versions I think it's worth explaining in the commit message.
>
> In patch 4/4 the value is limited to U32_MAX, but if you prefer I use a u32
> here instead, I can make that change.
Unless you have a clear reason not to, I think using u32 would be more
natural? If my head math is right the range for u32 is 4096 sec,
slightly over an hour? I'd use u32 and limit it to S32_MAX.
^ permalink raw reply
* Re: [PATCH net-next v6 4/4] eventpoll: Add epoll ioctl for epoll_params
From: Joe Damato @ 2024-02-07 19:16 UTC (permalink / raw)
To: Jakub Kicinski
Cc: netdev, linux-kernel, chuck.lever, jlayton, linux-api, brauner,
edumazet, davem, alexander.duyck, sridhar.samudrala,
willemdebruijn.kernel, weiwan, David.Laight, arnd, sdf,
amritha.nambiar, Jonathan Corbet, Alexander Viro, Jan Kara,
Nathan Lynch, Michael Ellerman, Greg Kroah-Hartman, Namjae Jeon,
Steve French, Thomas Zimmermann, Julien Panis, Andrew Waterman,
Palmer Dabbelt, Albert Ou, open list:DOCUMENTATION,
open list:FILESYSTEMS (VFS and infrastructure)
In-Reply-To: <20240207110726.68c07188@kernel.org>
On Wed, Feb 07, 2024 at 11:07:26AM -0800, Jakub Kicinski wrote:
> On Wed, 7 Feb 2024 10:50:15 -0800 Joe Damato wrote:
> > > This !! is unnecessary. Nonzero values shall be "converted" to true.
> > >
> > > But FWIW, the above is nothing which should be blocking, so:
> > ">
> > > Reviewed-by: Jiri Slaby <jirislaby@kernel.org>
> >
> > netdev maintainers: Jiri marked this with Reviewed-by, but was this review
> > what caused "Changes Requested" to be the status set for this patch set in
> > patchwork?
> >
> > If needed, I'll send a v7 with the changes Jiri suggested and add the
> > "Reviewed-by" since the changes are cosmetic, but I wanted to make sure
> > this was the reason.
>
> Yes, I think that's it.
OK, thanks for letting me know. I wasn't sure if it was because of the
netdev/source_inline which marked 1/4 as "fail" because of the inlines
added.
Does that need to be changed, as well?
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox