* [PATCH net-next v6 1/7] net: Add napi_struct parameter irq_suspend_timeout
2024-11-04 21:55 [PATCH net-next v6 0/7] Suspend IRQs during application busy periods Joe Damato
@ 2024-11-04 21:55 ` Joe Damato
2024-11-04 21:55 ` [PATCH net-next v6 2/7] net: Suspend softirq when prefer_busy_poll is set Joe Damato
` (5 subsequent siblings)
6 siblings, 0 replies; 17+ messages in thread
From: Joe Damato @ 2024-11-04 21:55 UTC (permalink / raw)
To: netdev
Cc: corbet, hdanton, bagasdotme, pabeni, namangulati, edumazet,
amritha.nambiar, sridhar.samudrala, sdf, peter, m2shafiei, bjorn,
hch, willy, willemdebruijn.kernel, skhawaja, kuba, Martin Karsten,
Joe Damato, Donald Hunter, David S. Miller, Simon Horman,
Andrew Lunn, Jesper Dangaard Brouer, Mina Almasry, Xuan Zhuo,
David Ahern, Sebastian Andrzej Siewior, Lorenzo Bianconi,
Alexander Lobakin, Jiri Pirko, Johannes Berg, open list
From: Martin Karsten <mkarsten@uwaterloo.ca>
Add a per-NAPI IRQ suspension parameter, which can be get/set with
netdev-genl.
This patch doesn't change any behavior but prepares the code for other
changes in the following commits which use irq_suspend_timeout as a
timeout for IRQ suspension.
Signed-off-by: Martin Karsten <mkarsten@uwaterloo.ca>
Co-developed-by: Joe Damato <jdamato@fastly.com>
Signed-off-by: Joe Damato <jdamato@fastly.com>
Tested-by: Joe Damato <jdamato@fastly.com>
Tested-by: Martin Karsten <mkarsten@uwaterloo.ca>
Acked-by: Stanislav Fomichev <sdf@fomichev.me>
Reviewed-by: Sridhar Samudrala <sridhar.samudrala@intel.com>
---
v1 -> v2:
- rewrote this patch to make irq_suspend_timeout per-napi via
netdev-genl.
rfc -> v1:
- removed napi.rst documentation from this patch; added to patch 6.
Documentation/netlink/specs/netdev.yaml | 7 +++++++
include/linux/netdevice.h | 2 ++
include/uapi/linux/netdev.h | 1 +
net/core/dev.c | 2 ++
net/core/dev.h | 25 +++++++++++++++++++++++++
net/core/netdev-genl-gen.c | 5 +++--
net/core/netdev-genl.c | 12 ++++++++++++
tools/include/uapi/linux/netdev.h | 1 +
8 files changed, 53 insertions(+), 2 deletions(-)
diff --git a/Documentation/netlink/specs/netdev.yaml b/Documentation/netlink/specs/netdev.yaml
index f9cb97d6106c..cbb544bd6c84 100644
--- a/Documentation/netlink/specs/netdev.yaml
+++ b/Documentation/netlink/specs/netdev.yaml
@@ -263,6 +263,11 @@ attribute-sets:
the end of a NAPI cycle. This may add receive latency in exchange
for reducing the number of frames processed by the network stack.
type: uint
+ -
+ name: irq-suspend-timeout
+ doc: The timeout, in nanoseconds, of how long to suspend irq
+ processing, if event polling finds events
+ type: uint
-
name: queue
attributes:
@@ -653,6 +658,7 @@ operations:
- pid
- defer-hard-irqs
- gro-flush-timeout
+ - irq-suspend-timeout
dump:
request:
attributes:
@@ -704,6 +710,7 @@ operations:
- id
- defer-hard-irqs
- gro-flush-timeout
+ - irq-suspend-timeout
kernel-family:
headers: [ "linux/list.h"]
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 3c552b648b27..c8ab5f08092b 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -347,6 +347,7 @@ struct gro_list {
*/
struct napi_config {
u64 gro_flush_timeout;
+ u64 irq_suspend_timeout;
u32 defer_hard_irqs;
unsigned int napi_id;
};
@@ -383,6 +384,7 @@ struct napi_struct {
struct hrtimer timer;
struct task_struct *thread;
unsigned long gro_flush_timeout;
+ unsigned long irq_suspend_timeout;
u32 defer_hard_irqs;
/* control-path-only fields follow */
struct list_head dev_list;
diff --git a/include/uapi/linux/netdev.h b/include/uapi/linux/netdev.h
index e3ebb49f60d2..e4be227d3ad6 100644
--- a/include/uapi/linux/netdev.h
+++ b/include/uapi/linux/netdev.h
@@ -124,6 +124,7 @@ enum {
NETDEV_A_NAPI_PID,
NETDEV_A_NAPI_DEFER_HARD_IRQS,
NETDEV_A_NAPI_GRO_FLUSH_TIMEOUT,
+ NETDEV_A_NAPI_IRQ_SUSPEND_TIMEOUT,
__NETDEV_A_NAPI_MAX,
NETDEV_A_NAPI_MAX = (__NETDEV_A_NAPI_MAX - 1)
diff --git a/net/core/dev.c b/net/core/dev.c
index 6a31152e4606..4d910872963f 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -6666,6 +6666,7 @@ static void napi_restore_config(struct napi_struct *n)
{
n->defer_hard_irqs = n->config->defer_hard_irqs;
n->gro_flush_timeout = n->config->gro_flush_timeout;
+ n->irq_suspend_timeout = n->config->irq_suspend_timeout;
/* a NAPI ID might be stored in the config, if so use it. if not, use
* napi_hash_add to generate one for us. It will be saved to the config
* in napi_disable.
@@ -6680,6 +6681,7 @@ static void napi_save_config(struct napi_struct *n)
{
n->config->defer_hard_irqs = n->defer_hard_irqs;
n->config->gro_flush_timeout = n->gro_flush_timeout;
+ n->config->irq_suspend_timeout = n->irq_suspend_timeout;
n->config->napi_id = n->napi_id;
napi_hash_del(n);
}
diff --git a/net/core/dev.h b/net/core/dev.h
index 7881bced70a9..d043dee25a68 100644
--- a/net/core/dev.h
+++ b/net/core/dev.h
@@ -236,6 +236,31 @@ static inline void netdev_set_gro_flush_timeout(struct net_device *netdev,
netdev->napi_config[i].gro_flush_timeout = timeout;
}
+/**
+ * napi_get_irq_suspend_timeout - get the irq_suspend_timeout
+ * @n: napi struct to get the irq_suspend_timeout from
+ *
+ * Return: the per-NAPI value of the irq_suspend_timeout field.
+ */
+static inline unsigned long
+napi_get_irq_suspend_timeout(const struct napi_struct *n)
+{
+ return READ_ONCE(n->irq_suspend_timeout);
+}
+
+/**
+ * napi_set_irq_suspend_timeout - set the irq_suspend_timeout for a napi
+ * @n: napi struct to set the irq_suspend_timeout
+ * @timeout: timeout value to set
+ *
+ * napi_set_irq_suspend_timeout sets the per-NAPI irq_suspend_timeout
+ */
+static inline void napi_set_irq_suspend_timeout(struct napi_struct *n,
+ unsigned long timeout)
+{
+ WRITE_ONCE(n->irq_suspend_timeout, timeout);
+}
+
int rps_cpumask_housekeeping(struct cpumask *mask);
#if defined(CONFIG_DEBUG_NET) && defined(CONFIG_BPF_SYSCALL)
diff --git a/net/core/netdev-genl-gen.c b/net/core/netdev-genl-gen.c
index 21de7e10be16..a89cbd8d87c3 100644
--- a/net/core/netdev-genl-gen.c
+++ b/net/core/netdev-genl-gen.c
@@ -92,10 +92,11 @@ static const struct nla_policy netdev_bind_rx_nl_policy[NETDEV_A_DMABUF_FD + 1]
};
/* NETDEV_CMD_NAPI_SET - do */
-static const struct nla_policy netdev_napi_set_nl_policy[NETDEV_A_NAPI_GRO_FLUSH_TIMEOUT + 1] = {
+static const struct nla_policy netdev_napi_set_nl_policy[NETDEV_A_NAPI_IRQ_SUSPEND_TIMEOUT + 1] = {
[NETDEV_A_NAPI_ID] = { .type = NLA_U32, },
[NETDEV_A_NAPI_DEFER_HARD_IRQS] = NLA_POLICY_FULL_RANGE(NLA_U32, &netdev_a_napi_defer_hard_irqs_range),
[NETDEV_A_NAPI_GRO_FLUSH_TIMEOUT] = { .type = NLA_UINT, },
+ [NETDEV_A_NAPI_IRQ_SUSPEND_TIMEOUT] = { .type = NLA_UINT, },
};
/* Ops table for netdev */
@@ -186,7 +187,7 @@ static const struct genl_split_ops netdev_nl_ops[] = {
.cmd = NETDEV_CMD_NAPI_SET,
.doit = netdev_nl_napi_set_doit,
.policy = netdev_napi_set_nl_policy,
- .maxattr = NETDEV_A_NAPI_GRO_FLUSH_TIMEOUT,
+ .maxattr = NETDEV_A_NAPI_IRQ_SUSPEND_TIMEOUT,
.flags = GENL_ADMIN_PERM | GENL_CMD_CAP_DO,
},
};
diff --git a/net/core/netdev-genl.c b/net/core/netdev-genl.c
index b49c3b4e5fbe..765ce7c9d73b 100644
--- a/net/core/netdev-genl.c
+++ b/net/core/netdev-genl.c
@@ -161,6 +161,7 @@ static int
netdev_nl_napi_fill_one(struct sk_buff *rsp, struct napi_struct *napi,
const struct genl_info *info)
{
+ unsigned long irq_suspend_timeout;
unsigned long gro_flush_timeout;
u32 napi_defer_hard_irqs;
void *hdr;
@@ -196,6 +197,11 @@ netdev_nl_napi_fill_one(struct sk_buff *rsp, struct napi_struct *napi,
napi_defer_hard_irqs))
goto nla_put_failure;
+ irq_suspend_timeout = napi_get_irq_suspend_timeout(napi);
+ if (nla_put_uint(rsp, NETDEV_A_NAPI_IRQ_SUSPEND_TIMEOUT,
+ irq_suspend_timeout))
+ goto nla_put_failure;
+
gro_flush_timeout = napi_get_gro_flush_timeout(napi);
if (nla_put_uint(rsp, NETDEV_A_NAPI_GRO_FLUSH_TIMEOUT,
gro_flush_timeout))
@@ -306,6 +312,7 @@ int netdev_nl_napi_get_dumpit(struct sk_buff *skb, struct netlink_callback *cb)
static int
netdev_nl_napi_set_config(struct napi_struct *napi, struct genl_info *info)
{
+ u64 irq_suspend_timeout = 0;
u64 gro_flush_timeout = 0;
u32 defer = 0;
@@ -314,6 +321,11 @@ netdev_nl_napi_set_config(struct napi_struct *napi, struct genl_info *info)
napi_set_defer_hard_irqs(napi, defer);
}
+ if (info->attrs[NETDEV_A_NAPI_IRQ_SUSPEND_TIMEOUT]) {
+ irq_suspend_timeout = nla_get_uint(info->attrs[NETDEV_A_NAPI_IRQ_SUSPEND_TIMEOUT]);
+ napi_set_irq_suspend_timeout(napi, irq_suspend_timeout);
+ }
+
if (info->attrs[NETDEV_A_NAPI_GRO_FLUSH_TIMEOUT]) {
gro_flush_timeout = nla_get_uint(info->attrs[NETDEV_A_NAPI_GRO_FLUSH_TIMEOUT]);
napi_set_gro_flush_timeout(napi, gro_flush_timeout);
diff --git a/tools/include/uapi/linux/netdev.h b/tools/include/uapi/linux/netdev.h
index e3ebb49f60d2..e4be227d3ad6 100644
--- a/tools/include/uapi/linux/netdev.h
+++ b/tools/include/uapi/linux/netdev.h
@@ -124,6 +124,7 @@ enum {
NETDEV_A_NAPI_PID,
NETDEV_A_NAPI_DEFER_HARD_IRQS,
NETDEV_A_NAPI_GRO_FLUSH_TIMEOUT,
+ NETDEV_A_NAPI_IRQ_SUSPEND_TIMEOUT,
__NETDEV_A_NAPI_MAX,
NETDEV_A_NAPI_MAX = (__NETDEV_A_NAPI_MAX - 1)
--
2.25.1
^ permalink raw reply related [flat|nested] 17+ messages in thread* [PATCH net-next v6 2/7] net: Suspend softirq when prefer_busy_poll is set
2024-11-04 21:55 [PATCH net-next v6 0/7] Suspend IRQs during application busy periods Joe Damato
2024-11-04 21:55 ` [PATCH net-next v6 1/7] net: Add napi_struct parameter irq_suspend_timeout Joe Damato
@ 2024-11-04 21:55 ` Joe Damato
2024-11-06 5:03 ` Jakub Kicinski
2024-11-04 21:55 ` [PATCH net-next v6 3/7] net: Add control functions for irq suspension Joe Damato
` (4 subsequent siblings)
6 siblings, 1 reply; 17+ messages in thread
From: Joe Damato @ 2024-11-04 21:55 UTC (permalink / raw)
To: netdev
Cc: corbet, hdanton, bagasdotme, pabeni, namangulati, edumazet,
amritha.nambiar, sridhar.samudrala, sdf, peter, m2shafiei, bjorn,
hch, willy, willemdebruijn.kernel, skhawaja, kuba, Martin Karsten,
Joe Damato, David S. Miller, Simon Horman, David Ahern,
Sebastian Andrzej Siewior, Lorenzo Bianconi, Alexander Lobakin,
open list
From: Martin Karsten <mkarsten@uwaterloo.ca>
When NAPI_F_PREFER_BUSY_POLL is set during busy_poll_stop and the
irq_suspend_timeout is nonzero, this timeout is used to defer softirq
scheduling, potentially longer than gro_flush_timeout. This can be used
to effectively suspend softirq processing during the time it takes for
an application to process data and return to the next busy-poll.
The call to napi->poll in busy_poll_stop might lead to an invocation of
napi_complete_done, but the prefer-busy flag is still set at that time,
so the same logic is used to defer softirq scheduling for
irq_suspend_timeout.
Signed-off-by: Martin Karsten <mkarsten@uwaterloo.ca>
Co-developed-by: Joe Damato <jdamato@fastly.com>
Signed-off-by: Joe Damato <jdamato@fastly.com>
Tested-by: Joe Damato <jdamato@fastly.com>
Tested-by: Martin Karsten <mkarsten@uwaterloo.ca>
Acked-by: Stanislav Fomichev <sdf@fomichev.me>
Reviewed-by: Sridhar Samudrala <sridhar.samudrala@intel.com>
---
v3:
- Removed reference to non-existent sysfs parameter from commit
message. No functional/code changes.
net/core/dev.c | 17 +++++++++++++----
1 file changed, 13 insertions(+), 4 deletions(-)
diff --git a/net/core/dev.c b/net/core/dev.c
index 4d910872963f..51d88f758e2e 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -6239,7 +6239,12 @@ bool napi_complete_done(struct napi_struct *n, int work_done)
timeout = napi_get_gro_flush_timeout(n);
n->defer_hard_irqs_count = napi_get_defer_hard_irqs(n);
}
- if (n->defer_hard_irqs_count > 0) {
+ if (napi_prefer_busy_poll(n)) {
+ timeout = napi_get_irq_suspend_timeout(n);
+ if (timeout)
+ ret = false;
+ }
+ if (ret && n->defer_hard_irqs_count > 0) {
n->defer_hard_irqs_count--;
timeout = napi_get_gro_flush_timeout(n);
if (timeout)
@@ -6375,9 +6380,13 @@ static void busy_poll_stop(struct napi_struct *napi, void *have_poll_lock,
bpf_net_ctx = bpf_net_ctx_set(&__bpf_net_ctx);
if (flags & NAPI_F_PREFER_BUSY_POLL) {
- napi->defer_hard_irqs_count = napi_get_defer_hard_irqs(napi);
- timeout = napi_get_gro_flush_timeout(napi);
- if (napi->defer_hard_irqs_count && timeout) {
+ timeout = napi_get_irq_suspend_timeout(napi);
+ if (!timeout) {
+ napi->defer_hard_irqs_count = napi_get_defer_hard_irqs(napi);
+ if (napi->defer_hard_irqs_count)
+ timeout = napi_get_gro_flush_timeout(napi);
+ }
+ if (timeout) {
hrtimer_start(&napi->timer, ns_to_ktime(timeout), HRTIMER_MODE_REL_PINNED);
skip_schedule = true;
}
--
2.25.1
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [PATCH net-next v6 2/7] net: Suspend softirq when prefer_busy_poll is set
2024-11-04 21:55 ` [PATCH net-next v6 2/7] net: Suspend softirq when prefer_busy_poll is set Joe Damato
@ 2024-11-06 5:03 ` Jakub Kicinski
2024-11-06 16:52 ` Joe Damato
0 siblings, 1 reply; 17+ messages in thread
From: Jakub Kicinski @ 2024-11-06 5:03 UTC (permalink / raw)
To: Joe Damato
Cc: netdev, corbet, hdanton, bagasdotme, pabeni, namangulati,
edumazet, amritha.nambiar, sridhar.samudrala, sdf, peter,
m2shafiei, bjorn, hch, willy, willemdebruijn.kernel, skhawaja,
Martin Karsten, David S. Miller, Simon Horman, David Ahern,
Sebastian Andrzej Siewior, Lorenzo Bianconi, Alexander Lobakin,
open list
On Mon, 4 Nov 2024 21:55:26 +0000 Joe Damato wrote:
> From: Martin Karsten <mkarsten@uwaterloo.ca>
>
> When NAPI_F_PREFER_BUSY_POLL is set during busy_poll_stop and the
> irq_suspend_timeout is nonzero, this timeout is used to defer softirq
> scheduling, potentially longer than gro_flush_timeout. This can be used
> to effectively suspend softirq processing during the time it takes for
> an application to process data and return to the next busy-poll.
>
> The call to napi->poll in busy_poll_stop might lead to an invocation of
The call to napi->poll when we're arming the timer is counter
productive, right? Maybe we can take this opportunity to add
the seemingly missing logic to skip over it?
> napi_complete_done, but the prefer-busy flag is still set at that time,
> so the same logic is used to defer softirq scheduling for
> irq_suspend_timeout.
>
> Signed-off-by: Martin Karsten <mkarsten@uwaterloo.ca>
> Co-developed-by: Joe Damato <jdamato@fastly.com>
> Signed-off-by: Joe Damato <jdamato@fastly.com>
> Tested-by: Joe Damato <jdamato@fastly.com>
> Tested-by: Martin Karsten <mkarsten@uwaterloo.ca>
> Acked-by: Stanislav Fomichev <sdf@fomichev.me>
> Reviewed-by: Sridhar Samudrala <sridhar.samudrala@intel.com>
> ---
> v3:
> - Removed reference to non-existent sysfs parameter from commit
> message. No functional/code changes.
>
> net/core/dev.c | 17 +++++++++++++----
> 1 file changed, 13 insertions(+), 4 deletions(-)
>
> diff --git a/net/core/dev.c b/net/core/dev.c
> index 4d910872963f..51d88f758e2e 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -6239,7 +6239,12 @@ bool napi_complete_done(struct napi_struct *n, int work_done)
> timeout = napi_get_gro_flush_timeout(n);
> n->defer_hard_irqs_count = napi_get_defer_hard_irqs(n);
> }
> - if (n->defer_hard_irqs_count > 0) {
> + if (napi_prefer_busy_poll(n)) {
> + timeout = napi_get_irq_suspend_timeout(n);
Why look at the suspend timeout in napi_complete_done()?
We are unlikely to be exiting busy poll here.
Is it because we need more time than gro_flush_timeout
for the application to take over the polling?
> + if (timeout)
> + ret = false;
> + }
> + if (ret && n->defer_hard_irqs_count > 0) {
> n->defer_hard_irqs_count--;
> timeout = napi_get_gro_flush_timeout(n);
> if (timeout)
> @@ -6375,9 +6380,13 @@ static void busy_poll_stop(struct napi_struct *napi, void *have_poll_lock,
> bpf_net_ctx = bpf_net_ctx_set(&__bpf_net_ctx);
>
> if (flags & NAPI_F_PREFER_BUSY_POLL) {
> - napi->defer_hard_irqs_count = napi_get_defer_hard_irqs(napi);
> - timeout = napi_get_gro_flush_timeout(napi);
> - if (napi->defer_hard_irqs_count && timeout) {
> + timeout = napi_get_irq_suspend_timeout(napi);
Even here I'm not sure if we need to trigger suspend.
I don't know the eventpoll code well but it seems like you suspend
and resume based on events when exiting epoll. Why also here?
> + if (!timeout) {
> + napi->defer_hard_irqs_count = napi_get_defer_hard_irqs(napi);
> + if (napi->defer_hard_irqs_count)
> + timeout = napi_get_gro_flush_timeout(napi);
> + }
> + if (timeout) {
> hrtimer_start(&napi->timer, ns_to_ktime(timeout), HRTIMER_MODE_REL_PINNED);
> skip_schedule = true;
> }
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH net-next v6 2/7] net: Suspend softirq when prefer_busy_poll is set
2024-11-06 5:03 ` Jakub Kicinski
@ 2024-11-06 16:52 ` Joe Damato
2024-11-06 23:31 ` Jakub Kicinski
0 siblings, 1 reply; 17+ messages in thread
From: Joe Damato @ 2024-11-06 16:52 UTC (permalink / raw)
To: Jakub Kicinski
Cc: netdev, corbet, hdanton, bagasdotme, pabeni, namangulati,
edumazet, amritha.nambiar, sridhar.samudrala, sdf, peter,
m2shafiei, bjorn, hch, willy, willemdebruijn.kernel, skhawaja,
Martin Karsten, David S. Miller, Simon Horman, David Ahern,
Sebastian Andrzej Siewior, Lorenzo Bianconi, Alexander Lobakin,
open list
On Tue, Nov 05, 2024 at 09:03:38PM -0800, Jakub Kicinski wrote:
> On Mon, 4 Nov 2024 21:55:26 +0000 Joe Damato wrote:
> > From: Martin Karsten <mkarsten@uwaterloo.ca>
> >
> > When NAPI_F_PREFER_BUSY_POLL is set during busy_poll_stop and the
> > irq_suspend_timeout is nonzero, this timeout is used to defer softirq
> > scheduling, potentially longer than gro_flush_timeout. This can be used
> > to effectively suspend softirq processing during the time it takes for
> > an application to process data and return to the next busy-poll.
> >
> > The call to napi->poll in busy_poll_stop might lead to an invocation of
>
> The call to napi->poll when we're arming the timer is counter
> productive, right? Maybe we can take this opportunity to add
> the seemingly missing logic to skip over it?
It seems like the call to napi->poll in busy_poll_stop is counter
productive and we're not opposed to making an optimization like that
in the future.
When we tried it, it triggered several bugs/system hangs, so we left
as much of the original code in place as possible.
The existing patch works and streamlining busy_poll_stop to skip the
call to napi->poll is an optimization that can be added as a later
series that focuses solely on when/where/how napi->poll is called.
Our focus was on:
- Not breaking any of the existing mechanisms
- Adding a new mechanism
I think we should avoid pulling the optimization you suggest into
this particular series and save that for the future.
> > napi_complete_done, but the prefer-busy flag is still set at that time,
> > so the same logic is used to defer softirq scheduling for
> > irq_suspend_timeout.
> >
> > Signed-off-by: Martin Karsten <mkarsten@uwaterloo.ca>
> > Co-developed-by: Joe Damato <jdamato@fastly.com>
> > Signed-off-by: Joe Damato <jdamato@fastly.com>
> > Tested-by: Joe Damato <jdamato@fastly.com>
> > Tested-by: Martin Karsten <mkarsten@uwaterloo.ca>
> > Acked-by: Stanislav Fomichev <sdf@fomichev.me>
> > Reviewed-by: Sridhar Samudrala <sridhar.samudrala@intel.com>
> > ---
> > v3:
> > - Removed reference to non-existent sysfs parameter from commit
> > message. No functional/code changes.
> >
> > net/core/dev.c | 17 +++++++++++++----
> > 1 file changed, 13 insertions(+), 4 deletions(-)
> >
> > diff --git a/net/core/dev.c b/net/core/dev.c
> > index 4d910872963f..51d88f758e2e 100644
> > --- a/net/core/dev.c
> > +++ b/net/core/dev.c
> > @@ -6239,7 +6239,12 @@ bool napi_complete_done(struct napi_struct *n, int work_done)
> > timeout = napi_get_gro_flush_timeout(n);
> > n->defer_hard_irqs_count = napi_get_defer_hard_irqs(n);
> > }
> > - if (n->defer_hard_irqs_count > 0) {
> > + if (napi_prefer_busy_poll(n)) {
> > + timeout = napi_get_irq_suspend_timeout(n);
>
> Why look at the suspend timeout in napi_complete_done()?
> We are unlikely to be exiting busy poll here.
The idea is similar to commit 7fd3253a7de6 ("net: Introduce
preferred busy-polling"); continue to defer IRQs as long as forward
progress is being made. In this case, napi->poll ran, called
napi_complete_done -- the system is moving forward with processing
so prevent IRQs from interrupting us.
epoll_wait will re-enable IRQs (by calling napi_schedule) if
there are no events ready for processing.
> Is it because we need more time than gro_flush_timeout
> for the application to take over the polling?
That's right; we want the application to retain control of packet
processing. That's why we connected this to the "prefer_busy_poll"
flag.
> > + if (timeout)
> > + ret = false;
> > + }
> > + if (ret && n->defer_hard_irqs_count > 0) {
> > n->defer_hard_irqs_count--;
> > timeout = napi_get_gro_flush_timeout(n);
> > if (timeout)
> > @@ -6375,9 +6380,13 @@ static void busy_poll_stop(struct napi_struct *napi, void *have_poll_lock,
> > bpf_net_ctx = bpf_net_ctx_set(&__bpf_net_ctx);
> >
> > if (flags & NAPI_F_PREFER_BUSY_POLL) {
> > - napi->defer_hard_irqs_count = napi_get_defer_hard_irqs(napi);
> > - timeout = napi_get_gro_flush_timeout(napi);
> > - if (napi->defer_hard_irqs_count && timeout) {
> > + timeout = napi_get_irq_suspend_timeout(napi);
>
> Even here I'm not sure if we need to trigger suspend.
> I don't know the eventpoll code well but it seems like you suspend
> and resume based on events when exiting epoll. Why also here?
There's two questions wrapped up here and an overall point to make:
1. Suspend and resume based on events when exiting epoll - that's
right and as you'll see in those patches that happens by:
- arming the suspend timer (via a call to napi_suspend_irqs)
when a positive number of events are retrieved
- calling napi_schedule (via napi_resume_irqs) when there are
no events or the epoll context is being freed.
2. Why defer the suspend timer here in busy_poll_stop? Note that the
original code would set the timer to gro_flush_timeout, which
would introduce the trade offs we mention in the cover letter
(latency for large values, IRQ interruption for small values).
We don't want the gro_flush_timeout to take over yet because we
want to avoid these tradeoffs up until the point where epoll_wait
finds no events for processing.
Does that make sense? If we skipped the IRQ suspend deferral
here, we'd be giving packet processing control back to
gro_flush_timeout and napi_defer_hard_irqs, but the system might
still have packets that can be processed in the next call to
epoll_wait.
The overall point to make is that: the suspend timer is used to
prevent misbehaving userland applications from taking too long. It's
essentially a backstop and, as long as the app is making forward
progress, allows the app to continue running its busy poll loop
undisturbed (via napi_complete_done preventing the driver from
enabling IRQs).
Does that make sense?
> > + if (!timeout) {
> > + napi->defer_hard_irqs_count = napi_get_defer_hard_irqs(napi);
> > + if (napi->defer_hard_irqs_count)
> > + timeout = napi_get_gro_flush_timeout(napi);
> > + }
> > + if (timeout) {
> > hrtimer_start(&napi->timer, ns_to_ktime(timeout), HRTIMER_MODE_REL_PINNED);
> > skip_schedule = true;
> > }
>
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH net-next v6 2/7] net: Suspend softirq when prefer_busy_poll is set
2024-11-06 16:52 ` Joe Damato
@ 2024-11-06 23:31 ` Jakub Kicinski
2024-11-07 3:24 ` Joe Damato
0 siblings, 1 reply; 17+ messages in thread
From: Jakub Kicinski @ 2024-11-06 23:31 UTC (permalink / raw)
To: Joe Damato
Cc: netdev, corbet, hdanton, bagasdotme, pabeni, namangulati,
edumazet, amritha.nambiar, sridhar.samudrala, sdf, peter,
m2shafiei, bjorn, hch, willy, willemdebruijn.kernel, skhawaja,
Martin Karsten, David S. Miller, Simon Horman, David Ahern,
Sebastian Andrzej Siewior, Lorenzo Bianconi, Alexander Lobakin,
open list
On Wed, 6 Nov 2024 08:52:00 -0800 Joe Damato wrote:
> On Tue, Nov 05, 2024 at 09:03:38PM -0800, Jakub Kicinski wrote:
> > On Mon, 4 Nov 2024 21:55:26 +0000 Joe Damato wrote:
> > > From: Martin Karsten <mkarsten@uwaterloo.ca>
> > >
> > > When NAPI_F_PREFER_BUSY_POLL is set during busy_poll_stop and the
> > > irq_suspend_timeout is nonzero, this timeout is used to defer softirq
> > > scheduling, potentially longer than gro_flush_timeout. This can be used
> > > to effectively suspend softirq processing during the time it takes for
> > > an application to process data and return to the next busy-poll.
> > >
> > > The call to napi->poll in busy_poll_stop might lead to an invocation of
> >
> > The call to napi->poll when we're arming the timer is counter
> > productive, right? Maybe we can take this opportunity to add
> > the seemingly missing logic to skip over it?
>
> It seems like the call to napi->poll in busy_poll_stop is counter
> productive and we're not opposed to making an optimization like that
> in the future.
>
> When we tried it, it triggered several bugs/system hangs, so we left
> as much of the original code in place as possible.
You don't happen to have the patch you used? Many ways to get the
skipping wrong.
> The existing patch works and streamlining busy_poll_stop to skip the
> call to napi->poll is an optimization that can be added as a later
> series that focuses solely on when/where/how napi->poll is called.
The reason I brought it up is that it rearms the timer, if driver
ends up calling napi_complete_done(). So we arm the timer in
napi_poll_stop(), then call the driver which may rearm again,
making the already complex code even harder to reason about.
> Our focus was on:
> - Not breaking any of the existing mechanisms
> - Adding a new mechanism
>
> I think we should avoid pulling the optimization you suggest into
> this particular series and save that for the future.
I'm primarily worried about maintainability of this code.
> > > diff --git a/net/core/dev.c b/net/core/dev.c
> > > index 4d910872963f..51d88f758e2e 100644
> > > --- a/net/core/dev.c
> > > +++ b/net/core/dev.c
> > > @@ -6239,7 +6239,12 @@ bool napi_complete_done(struct napi_struct *n, int work_done)
> > > timeout = napi_get_gro_flush_timeout(n);
> > > n->defer_hard_irqs_count = napi_get_defer_hard_irqs(n);
> > > }
> > > - if (n->defer_hard_irqs_count > 0) {
> > > + if (napi_prefer_busy_poll(n)) {
> > > + timeout = napi_get_irq_suspend_timeout(n);
> >
> > Why look at the suspend timeout in napi_complete_done()?
> > We are unlikely to be exiting busy poll here.
>
> The idea is similar to commit 7fd3253a7de6 ("net: Introduce
> preferred busy-polling"); continue to defer IRQs as long as forward
> progress is being made. In this case, napi->poll ran, called
> napi_complete_done -- the system is moving forward with processing
> so prevent IRQs from interrupting us.
We should clarify the mental models. You're describing IRQ deferal,
but say prefer busy poll.
Prefer busy poll has only one function - if we are at 100% busy
and always see >= budget of packets on the ring, we never call
napi_complete_done(). Drivers don't call napi_complete_done() if they
consumed full budget. So we need a way to break that re-polling loop,
release the NAPI ownership and give busy poll a chance to claim the
NAPI instance ownership (the SCHED bit). We check for prefer
busy poll in __napi_poll(), because, again, in the target scenario
napi_complete_done() is never called.
The IRQ deferal mechanism is necessary for prefer busy poll to work,
but it's separate and used by some drivers without good IRQ coalescing,
no user space polling involved.
In your case, when machine is not melting under 100% load - prefer busy
poll will be set once or not at all.
> epoll_wait will re-enable IRQs (by calling napi_schedule) if
> there are no events ready for processing.
To be 100% precise calling napi_schedule will not reenable IRQs
if IRQ deferal is active. It only guarantees one NAPI run in
softirq (or thread if threaded).
> > Is it because we need more time than gro_flush_timeout
> > for the application to take over the polling?
>
> That's right; we want the application to retain control of packet
> processing. That's why we connected this to the "prefer_busy_poll"
> flag.
>
> > > + if (timeout)
> > > + ret = false;
> > > + }
> > > + if (ret && n->defer_hard_irqs_count > 0) {
> > > n->defer_hard_irqs_count--;
> > > timeout = napi_get_gro_flush_timeout(n);
> > > if (timeout)
> > > @@ -6375,9 +6380,13 @@ static void busy_poll_stop(struct napi_struct *napi, void *have_poll_lock,
> > > bpf_net_ctx = bpf_net_ctx_set(&__bpf_net_ctx);
> > >
> > > if (flags & NAPI_F_PREFER_BUSY_POLL) {
> > > - napi->defer_hard_irqs_count = napi_get_defer_hard_irqs(napi);
> > > - timeout = napi_get_gro_flush_timeout(napi);
> > > - if (napi->defer_hard_irqs_count && timeout) {
> > > + timeout = napi_get_irq_suspend_timeout(napi);
> >
> > Even here I'm not sure if we need to trigger suspend.
> > I don't know the eventpoll code well but it seems like you suspend
> > and resume based on events when exiting epoll. Why also here?
>
> There's two questions wrapped up here and an overall point to make:
>
> 1. Suspend and resume based on events when exiting epoll - that's
> right and as you'll see in those patches that happens by:
> - arming the suspend timer (via a call to napi_suspend_irqs)
> when a positive number of events are retrieved
> - calling napi_schedule (via napi_resume_irqs) when there are
> no events or the epoll context is being freed.
>
> 2. Why defer the suspend timer here in busy_poll_stop? Note that the
> original code would set the timer to gro_flush_timeout, which
> would introduce the trade offs we mention in the cover letter
> (latency for large values, IRQ interruption for small values).
>
> We don't want the gro_flush_timeout to take over yet because we
> want to avoid these tradeoffs up until the point where epoll_wait
> finds no events for processing.
>
> Does that make sense? If we skipped the IRQ suspend deferral
> here, we'd be giving packet processing control back to
> gro_flush_timeout and napi_defer_hard_irqs, but the system might
> still have packets that can be processed in the next call to
> epoll_wait.
Let me tell you what I think happens and then you can correct me.
0 epoll
1 # ..does its magic..
2 __napi_busy_loop()
3 # finds a waking packet
4 busy_poll_stop()
5 # arms the timer for long suspend
6 # epoll sees events
7 ep_suspend_napi_irqs()
8 napi_suspend_irqs()
9 # arms for long timer again
The timer we arm here only has to survive from line 5 to line 9,
because we will override the timeout on line 9.
> The overall point to make is that: the suspend timer is used to
> prevent misbehaving userland applications from taking too long. It's
> essentially a backstop and, as long as the app is making forward
> progress, allows the app to continue running its busy poll loop
> undisturbed (via napi_complete_done preventing the driver from
> enabling IRQs).
>
> Does that make sense?
My mental model put in yet another way is that only epoll knows if it
has events, and therefore whether the timeout should be short or long.
So the suspend timer should only be applied by epoll.
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH net-next v6 2/7] net: Suspend softirq when prefer_busy_poll is set
2024-11-06 23:31 ` Jakub Kicinski
@ 2024-11-07 3:24 ` Joe Damato
2024-11-07 21:01 ` Joe Damato
2024-11-08 3:52 ` Jakub Kicinski
0 siblings, 2 replies; 17+ messages in thread
From: Joe Damato @ 2024-11-07 3:24 UTC (permalink / raw)
To: Jakub Kicinski
Cc: netdev, corbet, hdanton, bagasdotme, pabeni, namangulati,
edumazet, amritha.nambiar, sridhar.samudrala, sdf, peter,
m2shafiei, bjorn, hch, willy, willemdebruijn.kernel, skhawaja,
Martin Karsten, David S. Miller, Simon Horman, David Ahern,
Sebastian Andrzej Siewior, Lorenzo Bianconi, Alexander Lobakin,
open list
On Wed, Nov 06, 2024 at 03:31:00PM -0800, Jakub Kicinski wrote:
> On Wed, 6 Nov 2024 08:52:00 -0800 Joe Damato wrote:
> > On Tue, Nov 05, 2024 at 09:03:38PM -0800, Jakub Kicinski wrote:
> > > On Mon, 4 Nov 2024 21:55:26 +0000 Joe Damato wrote:
> > > > From: Martin Karsten <mkarsten@uwaterloo.ca>
> > > >
> > > > When NAPI_F_PREFER_BUSY_POLL is set during busy_poll_stop and the
> > > > irq_suspend_timeout is nonzero, this timeout is used to defer softirq
> > > > scheduling, potentially longer than gro_flush_timeout. This can be used
> > > > to effectively suspend softirq processing during the time it takes for
> > > > an application to process data and return to the next busy-poll.
> > > >
> > > > The call to napi->poll in busy_poll_stop might lead to an invocation of
> > >
> > > The call to napi->poll when we're arming the timer is counter
> > > productive, right? Maybe we can take this opportunity to add
> > > the seemingly missing logic to skip over it?
> >
> > It seems like the call to napi->poll in busy_poll_stop is counter
> > productive and we're not opposed to making an optimization like that
> > in the future.
> >
> > When we tried it, it triggered several bugs/system hangs, so we left
> > as much of the original code in place as possible.
>
> You don't happen to have the patch you used? Many ways to get the
> skipping wrong.
Please see below; I think we've found a solution.
> > The existing patch works and streamlining busy_poll_stop to skip the
> > call to napi->poll is an optimization that can be added as a later
> > series that focuses solely on when/where/how napi->poll is called.
>
> The reason I brought it up is that it rearms the timer, if driver
> ends up calling napi_complete_done(). So we arm the timer in
> napi_poll_stop(), then call the driver which may rearm again,
> making the already complex code even harder to reason about.
Agreed that the timer is unnecessarily re-armed twice.
Martin ran some initial tests of this series but with this patch
(patch 2) dropped and the initial results from a small number of
runs seem fine.
In other words: I think we can simply drop this patch entirely,
re-run our tests to regenerate the data, update the documentation,
and send a v7.
But please continue reading below.
> > Our focus was on:
> > - Not breaking any of the existing mechanisms
> > - Adding a new mechanism
> >
> > I think we should avoid pulling the optimization you suggest into
> > this particular series and save that for the future.
>
> I'm primarily worried about maintainability of this code.
Of course and we're open to figuring out how to help with that.
> > > > diff --git a/net/core/dev.c b/net/core/dev.c
> > > > index 4d910872963f..51d88f758e2e 100644
> > > > --- a/net/core/dev.c
> > > > +++ b/net/core/dev.c
> > > > @@ -6239,7 +6239,12 @@ bool napi_complete_done(struct napi_struct *n, int work_done)
> > > > timeout = napi_get_gro_flush_timeout(n);
> > > > n->defer_hard_irqs_count = napi_get_defer_hard_irqs(n);
> > > > }
> > > > - if (n->defer_hard_irqs_count > 0) {
> > > > + if (napi_prefer_busy_poll(n)) {
> > > > + timeout = napi_get_irq_suspend_timeout(n);
> > >
> > > Why look at the suspend timeout in napi_complete_done()?
> > > We are unlikely to be exiting busy poll here.
> >
> > The idea is similar to commit 7fd3253a7de6 ("net: Introduce
> > preferred busy-polling"); continue to defer IRQs as long as forward
> > progress is being made. In this case, napi->poll ran, called
> > napi_complete_done -- the system is moving forward with processing
> > so prevent IRQs from interrupting us.
>
> We should clarify the mental models. You're describing IRQ deferal,
> but say prefer busy poll.
OK; we're open to using different language if that would be helpful.
> Prefer busy poll has only one function - if we are at 100% busy
> and always see >= budget of packets on the ring, we never call
> napi_complete_done(). Drivers don't call napi_complete_done() if they
> consumed full budget. So we need a way to break that re-polling loop,
> release the NAPI ownership and give busy poll a chance to claim the
> NAPI instance ownership (the SCHED bit). We check for prefer
> busy poll in __napi_poll(), because, again, in the target scenario
> napi_complete_done() is never called.
>
> The IRQ deferal mechanism is necessary for prefer busy poll to work,
> but it's separate and used by some drivers without good IRQ coalescing,
> no user space polling involved.
Sure, we agree and are on the same page on the above about what
prefer busy poll is and its interaction with IRQ deferral.
> In your case, when machine is not melting under 100% load - prefer busy
> poll will be set once or not at all.
I am not sure what you mean by that last sentence, because the
prefer busy poll flag is set by the application?
Similar to prefer busy poll piggybacking on IRQ deferral, we
piggyback on prefer busy polling by allowing the application to use
an even larger timeout while it is processing incoming data, i.e.,
deferring IRQs for a longer period, but only after a successful busy
poll. This makes prefer busy poll + irq suspend useful when
utilization is below 100%.
> > epoll_wait will re-enable IRQs (by calling napi_schedule) if
> > there are no events ready for processing.
>
> To be 100% precise calling napi_schedule will not reenable IRQs
> if IRQ deferal is active. It only guarantees one NAPI run in
> softirq (or thread if threaded).
Yes, I should have been more precise.
Calling napi_schedule doesn't enable IRQs, but runs NAPI which sets
about the process of *eventually* causing napi_complete_done to
return true which triggers the re-arming of IRQs by the driver.
> > > Is it because we need more time than gro_flush_timeout
> > > for the application to take over the polling?
> >
> > That's right; we want the application to retain control of packet
> > processing. That's why we connected this to the "prefer_busy_poll"
> > flag.
> >
> > > > + if (timeout)
> > > > + ret = false;
> > > > + }
> > > > + if (ret && n->defer_hard_irqs_count > 0) {
> > > > n->defer_hard_irqs_count--;
> > > > timeout = napi_get_gro_flush_timeout(n);
> > > > if (timeout)
> > > > @@ -6375,9 +6380,13 @@ static void busy_poll_stop(struct napi_struct *napi, void *have_poll_lock,
> > > > bpf_net_ctx = bpf_net_ctx_set(&__bpf_net_ctx);
> > > >
> > > > if (flags & NAPI_F_PREFER_BUSY_POLL) {
> > > > - napi->defer_hard_irqs_count = napi_get_defer_hard_irqs(napi);
> > > > - timeout = napi_get_gro_flush_timeout(napi);
> > > > - if (napi->defer_hard_irqs_count && timeout) {
> > > > + timeout = napi_get_irq_suspend_timeout(napi);
> > >
> > > Even here I'm not sure if we need to trigger suspend.
> > > I don't know the eventpoll code well but it seems like you suspend
> > > and resume based on events when exiting epoll. Why also here?
> >
> > There's two questions wrapped up here and an overall point to make:
> >
> > 1. Suspend and resume based on events when exiting epoll - that's
> > right and as you'll see in those patches that happens by:
> > - arming the suspend timer (via a call to napi_suspend_irqs)
> > when a positive number of events are retrieved
> > - calling napi_schedule (via napi_resume_irqs) when there are
> > no events or the epoll context is being freed.
> >
> > 2. Why defer the suspend timer here in busy_poll_stop? Note that the
> > original code would set the timer to gro_flush_timeout, which
> > would introduce the trade offs we mention in the cover letter
> > (latency for large values, IRQ interruption for small values).
> >
> > We don't want the gro_flush_timeout to take over yet because we
> > want to avoid these tradeoffs up until the point where epoll_wait
> > finds no events for processing.
> >
> > Does that make sense? If we skipped the IRQ suspend deferral
> > here, we'd be giving packet processing control back to
> > gro_flush_timeout and napi_defer_hard_irqs, but the system might
> > still have packets that can be processed in the next call to
> > epoll_wait.
>
> Let me tell you what I think happens and then you can correct me.
>
> 0 epoll
> 1 # ..does its magic..
> 2 __napi_busy_loop()
> 3 # finds a waking packet
> 4 busy_poll_stop()
> 5 # arms the timer for long suspend
> 6 # epoll sees events
> 7 ep_suspend_napi_irqs()
> 8 napi_suspend_irqs()
> 9 # arms for long timer again
>
> The timer we arm here only has to survive from line 5 to line 9,
> because we will override the timeout on line 9.
Yes, you are right. Thanks for highlighting and catching this.
> > The overall point to make is that: the suspend timer is used to
> > prevent misbehaving userland applications from taking too long. It's
> > essentially a backstop and, as long as the app is making forward
> > progress, allows the app to continue running its busy poll loop
> > undisturbed (via napi_complete_done preventing the driver from
> > enabling IRQs).
> >
> > Does that make sense?
>
> My mental model put in yet another way is that only epoll knows if it
> has events, and therefore whether the timeout should be short or long.
> So the suspend timer should only be applied by epoll.
Here's what we are thinking, can you let me know if you agree with
this?
- We can drop patch 2 entirely
- Update the documentation about IRQ suspension as needed now
that patch 2 has been dropped
- Leave the rest of the series as is
- Re-run our tests to gather sufficient data for the test cases
outlined in the cover letter to ensure that the performance
numbers hold over several iterations
Does that seem reasonable for the v7 to you?
I am asking because generating the amount of data over the number of
scenarios we are testing takes a long time and I want to make sure
we are as aligned as we can be before I kick off another run :)
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH net-next v6 2/7] net: Suspend softirq when prefer_busy_poll is set
2024-11-07 3:24 ` Joe Damato
@ 2024-11-07 21:01 ` Joe Damato
2024-11-08 3:52 ` Jakub Kicinski
1 sibling, 0 replies; 17+ messages in thread
From: Joe Damato @ 2024-11-07 21:01 UTC (permalink / raw)
To: Jakub Kicinski, netdev, corbet, hdanton, bagasdotme, pabeni,
namangulati, edumazet, amritha.nambiar, sridhar.samudrala, sdf,
peter, m2shafiei, bjorn, hch, willy, willemdebruijn.kernel,
skhawaja, Martin Karsten, David S. Miller, Simon Horman,
David Ahern, Sebastian Andrzej Siewior, Lorenzo Bianconi,
Alexander Lobakin, open list
On Wed, Nov 06, 2024 at 07:24:32PM -0800, Joe Damato wrote:
> On Wed, Nov 06, 2024 at 03:31:00PM -0800, Jakub Kicinski wrote:
> > On Wed, 6 Nov 2024 08:52:00 -0800 Joe Damato wrote:
> > > On Tue, Nov 05, 2024 at 09:03:38PM -0800, Jakub Kicinski wrote:
> > > > On Mon, 4 Nov 2024 21:55:26 +0000 Joe Damato wrote:
[...]
> > 0 epoll
> > 1 # ..does its magic..
> > 2 __napi_busy_loop()
> > 3 # finds a waking packet
> > 4 busy_poll_stop()
> > 5 # arms the timer for long suspend
> > 6 # epoll sees events
> > 7 ep_suspend_napi_irqs()
> > 8 napi_suspend_irqs()
> > 9 # arms for long timer again
> >
> > The timer we arm here only has to survive from line 5 to line 9,
> > because we will override the timeout on line 9.
>
> Yes, you are right. Thanks for highlighting and catching this.
>
> > > The overall point to make is that: the suspend timer is used to
> > > prevent misbehaving userland applications from taking too long. It's
> > > essentially a backstop and, as long as the app is making forward
> > > progress, allows the app to continue running its busy poll loop
> > > undisturbed (via napi_complete_done preventing the driver from
> > > enabling IRQs).
> > >
> > > Does that make sense?
> >
> > My mental model put in yet another way is that only epoll knows if it
> > has events, and therefore whether the timeout should be short or long.
> > So the suspend timer should only be applied by epoll.
>
> Here's what we are thinking, can you let me know if you agree with
> this?
>
> - We can drop patch 2 entirely
> - Update the documentation about IRQ suspension as needed now
> that patch 2 has been dropped
> - Leave the rest of the series as is
> - Re-run our tests to gather sufficient data for the test cases
> outlined in the cover letter to ensure that the performance
> numbers hold over several iterations
>
> Does that seem reasonable for the v7 to you?
>
> I am asking because generating the amount of data over the number of
> scenarios we are testing takes a long time and I want to make sure
> we are as aligned as we can be before I kick off another run :)
I just noticed this was marked "changes requested". I re-ran the
tests overnight and have the data to confirm results are the same
even after dropping patch 2, which simplifies the code and removes
the double arming of the timer.
I wasn't sure if you were asking for other changes other than
dropping patch 2, but since I have the data I'm going to proceed as
specified in my previous email above:
- Drop patch 2
- Update cover letter with new data
- Send that as v7
Unless you'd like me to hold off for some reason? Or if there was
some other feedback I need to address that I am missing?
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH net-next v6 2/7] net: Suspend softirq when prefer_busy_poll is set
2024-11-07 3:24 ` Joe Damato
2024-11-07 21:01 ` Joe Damato
@ 2024-11-08 3:52 ` Jakub Kicinski
1 sibling, 0 replies; 17+ messages in thread
From: Jakub Kicinski @ 2024-11-08 3:52 UTC (permalink / raw)
To: Joe Damato
Cc: netdev, corbet, hdanton, bagasdotme, pabeni, namangulati,
edumazet, amritha.nambiar, sridhar.samudrala, sdf, peter,
m2shafiei, bjorn, hch, willy, willemdebruijn.kernel, skhawaja,
Martin Karsten, David S. Miller, Simon Horman, David Ahern,
Sebastian Andrzej Siewior, Lorenzo Bianconi, Alexander Lobakin,
open list
On Wed, 6 Nov 2024 19:24:32 -0800 Joe Damato wrote:
> > In your case, when machine is not melting under 100% load - prefer busy
> > poll will be set once or not at all.
>
> I am not sure what you mean by that last sentence, because the
> prefer busy poll flag is set by the application?
There are two flags with almost exactly the same name, that's probably
the source of misunderstanding. NAPI_F_PREFER_BUSY_POLL will always be
set, but unless we are actually in a "napi live lock"
NAPI_STATE_PREFER_BUSY_POLL will not get set, and the latter is what
napi_prefer_busy_poll() tests. But we're dropping this patch so
doesn't matter. I was trying to pile up reasons why checking
napi_prefer_busy_poll() should not be necessary.
> Similar to prefer busy poll piggybacking on IRQ deferral, we
> piggyback on prefer busy polling by allowing the application to use
> an even larger timeout while it is processing incoming data, i.e.,
> deferring IRQs for a longer period, but only after a successful busy
> poll. This makes prefer busy poll + irq suspend useful when
> utilization is below 100%.
>
> > > The overall point to make is that: the suspend timer is used to
> > > prevent misbehaving userland applications from taking too long. It's
> > > essentially a backstop and, as long as the app is making forward
> > > progress, allows the app to continue running its busy poll loop
> > > undisturbed (via napi_complete_done preventing the driver from
> > > enabling IRQs).
> > >
> > > Does that make sense?
> >
> > My mental model put in yet another way is that only epoll knows if it
> > has events, and therefore whether the timeout should be short or long.
> > So the suspend timer should only be applied by epoll.
>
> Here's what we are thinking, can you let me know if you agree with
> this?
>
> - We can drop patch 2 entirely
> - Update the documentation about IRQ suspension as needed now
> that patch 2 has been dropped
> - Leave the rest of the series as is
> - Re-run our tests to gather sufficient data for the test cases
> outlined in the cover letter to ensure that the performance
> numbers hold over several iterations
>
> Does that seem reasonable for the v7 to you?
>
> I am asking because generating the amount of data over the number of
> scenarios we are testing takes a long time and I want to make sure
> we are as aligned as we can be before I kick off another run :)
SGTM, the rest of the series makes perfect sense to me.
Sorry for the delay..
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH net-next v6 3/7] net: Add control functions for irq suspension
2024-11-04 21:55 [PATCH net-next v6 0/7] Suspend IRQs during application busy periods Joe Damato
2024-11-04 21:55 ` [PATCH net-next v6 1/7] net: Add napi_struct parameter irq_suspend_timeout Joe Damato
2024-11-04 21:55 ` [PATCH net-next v6 2/7] net: Suspend softirq when prefer_busy_poll is set Joe Damato
@ 2024-11-04 21:55 ` Joe Damato
2024-11-04 21:55 ` [PATCH net-next v6 4/7] eventpoll: Trigger napi_busy_loop, if prefer_busy_poll is set Joe Damato
` (3 subsequent siblings)
6 siblings, 0 replies; 17+ messages in thread
From: Joe Damato @ 2024-11-04 21:55 UTC (permalink / raw)
To: netdev
Cc: corbet, hdanton, bagasdotme, pabeni, namangulati, edumazet,
amritha.nambiar, sridhar.samudrala, sdf, peter, m2shafiei, bjorn,
hch, willy, willemdebruijn.kernel, skhawaja, kuba, Martin Karsten,
Joe Damato, David S. Miller, Simon Horman, David Ahern,
Sebastian Andrzej Siewior, Lorenzo Bianconi, Alexander Lobakin,
open list
From: Martin Karsten <mkarsten@uwaterloo.ca>
The napi_suspend_irqs routine bootstraps irq suspension by elongating
the defer timeout to irq_suspend_timeout.
The napi_resume_irqs routine effectively cancels irq suspension by
forcing the napi to be scheduled immediately.
Signed-off-by: Martin Karsten <mkarsten@uwaterloo.ca>
Co-developed-by: Joe Damato <jdamato@fastly.com>
Signed-off-by: Joe Damato <jdamato@fastly.com>
Tested-by: Joe Damato <jdamato@fastly.com>
Tested-by: Martin Karsten <mkarsten@uwaterloo.ca>
Acked-by: Stanislav Fomichev <sdf@fomichev.me>
Reviewed-by: Sridhar Samudrala <sridhar.samudrala@intel.com>
---
v1 -> v2:
- Added a comment to napi_resume_irqs.
include/net/busy_poll.h | 3 +++
net/core/dev.c | 39 +++++++++++++++++++++++++++++++++++++++
2 files changed, 42 insertions(+)
diff --git a/include/net/busy_poll.h b/include/net/busy_poll.h
index f03040baaefd..c858270141bc 100644
--- a/include/net/busy_poll.h
+++ b/include/net/busy_poll.h
@@ -52,6 +52,9 @@ void napi_busy_loop_rcu(unsigned int napi_id,
bool (*loop_end)(void *, unsigned long),
void *loop_end_arg, bool prefer_busy_poll, u16 budget);
+void napi_suspend_irqs(unsigned int napi_id);
+void napi_resume_irqs(unsigned int napi_id);
+
#else /* CONFIG_NET_RX_BUSY_POLL */
static inline unsigned long net_busy_loop_on(void)
{
diff --git a/net/core/dev.c b/net/core/dev.c
index 51d88f758e2e..9d903ce0c2b0 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -6516,6 +6516,45 @@ void napi_busy_loop(unsigned int napi_id,
}
EXPORT_SYMBOL(napi_busy_loop);
+void napi_suspend_irqs(unsigned int napi_id)
+{
+ struct napi_struct *napi;
+
+ rcu_read_lock();
+ napi = napi_by_id(napi_id);
+ if (napi) {
+ unsigned long timeout = napi_get_irq_suspend_timeout(napi);
+
+ if (timeout)
+ hrtimer_start(&napi->timer, ns_to_ktime(timeout),
+ HRTIMER_MODE_REL_PINNED);
+ }
+ rcu_read_unlock();
+}
+EXPORT_SYMBOL(napi_suspend_irqs);
+
+void napi_resume_irqs(unsigned int napi_id)
+{
+ struct napi_struct *napi;
+
+ rcu_read_lock();
+ napi = napi_by_id(napi_id);
+ if (napi) {
+ /* If irq_suspend_timeout is set to 0 between the call to
+ * napi_suspend_irqs and now, the original value still
+ * determines the safety timeout as intended and napi_watchdog
+ * will resume irq processing.
+ */
+ if (napi_get_irq_suspend_timeout(napi)) {
+ local_bh_disable();
+ napi_schedule(napi);
+ local_bh_enable();
+ }
+ }
+ rcu_read_unlock();
+}
+EXPORT_SYMBOL(napi_resume_irqs);
+
#endif /* CONFIG_NET_RX_BUSY_POLL */
static void __napi_hash_add_with_id(struct napi_struct *napi,
--
2.25.1
^ permalink raw reply related [flat|nested] 17+ messages in thread* [PATCH net-next v6 4/7] eventpoll: Trigger napi_busy_loop, if prefer_busy_poll is set
2024-11-04 21:55 [PATCH net-next v6 0/7] Suspend IRQs during application busy periods Joe Damato
` (2 preceding siblings ...)
2024-11-04 21:55 ` [PATCH net-next v6 3/7] net: Add control functions for irq suspension Joe Damato
@ 2024-11-04 21:55 ` Joe Damato
2024-11-04 21:55 ` [PATCH net-next v6 5/7] eventpoll: Control irq suspension for prefer_busy_poll Joe Damato
` (2 subsequent siblings)
6 siblings, 0 replies; 17+ messages in thread
From: Joe Damato @ 2024-11-04 21:55 UTC (permalink / raw)
To: netdev
Cc: corbet, hdanton, bagasdotme, pabeni, namangulati, edumazet,
amritha.nambiar, sridhar.samudrala, sdf, peter, m2shafiei, bjorn,
hch, willy, willemdebruijn.kernel, skhawaja, kuba, Martin Karsten,
Joe Damato, Alexander Viro, Christian Brauner, Jan Kara,
open list:FILESYSTEMS (VFS and infrastructure), open list
From: Martin Karsten <mkarsten@uwaterloo.ca>
Setting prefer_busy_poll now leads to an effectively nonblocking
iteration though napi_busy_loop, even when busy_poll_usecs is 0.
Signed-off-by: Martin Karsten <mkarsten@uwaterloo.ca>
Co-developed-by: Joe Damato <jdamato@fastly.com>
Signed-off-by: Joe Damato <jdamato@fastly.com>
Tested-by: Joe Damato <jdamato@fastly.com>
Tested-by: Martin Karsten <mkarsten@uwaterloo.ca>
Acked-by: Stanislav Fomichev <sdf@fomichev.me>
Reviewed-by: Sridhar Samudrala <sridhar.samudrala@intel.com>
---
v1 -> v2:
- Rebased to apply now that commit b9ca079dd6b0 ("eventpoll: Annotate
data-race of busy_poll_usecs") has been picked up from VFS.
fs/eventpoll.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/fs/eventpoll.c b/fs/eventpoll.c
index 1ae4542f0bd8..f9e0d9307dad 100644
--- a/fs/eventpoll.c
+++ b/fs/eventpoll.c
@@ -420,7 +420,9 @@ static bool busy_loop_ep_timeout(unsigned long start_time,
static bool ep_busy_loop_on(struct eventpoll *ep)
{
- return !!READ_ONCE(ep->busy_poll_usecs) || net_busy_loop_on();
+ return !!READ_ONCE(ep->busy_poll_usecs) ||
+ READ_ONCE(ep->prefer_busy_poll) ||
+ net_busy_loop_on();
}
static bool ep_busy_loop_end(void *p, unsigned long start_time)
--
2.25.1
^ permalink raw reply related [flat|nested] 17+ messages in thread* [PATCH net-next v6 5/7] eventpoll: Control irq suspension for prefer_busy_poll
2024-11-04 21:55 [PATCH net-next v6 0/7] Suspend IRQs during application busy periods Joe Damato
` (3 preceding siblings ...)
2024-11-04 21:55 ` [PATCH net-next v6 4/7] eventpoll: Trigger napi_busy_loop, if prefer_busy_poll is set Joe Damato
@ 2024-11-04 21:55 ` Joe Damato
2024-11-04 21:55 ` [PATCH net-next v6 6/7] selftests: net: Add busy_poll_test Joe Damato
2024-11-04 21:55 ` [PATCH net-next v6 7/7] docs: networking: Describe irq suspension Joe Damato
6 siblings, 0 replies; 17+ messages in thread
From: Joe Damato @ 2024-11-04 21:55 UTC (permalink / raw)
To: netdev
Cc: corbet, hdanton, bagasdotme, pabeni, namangulati, edumazet,
amritha.nambiar, sridhar.samudrala, sdf, peter, m2shafiei, bjorn,
hch, willy, willemdebruijn.kernel, skhawaja, kuba, Martin Karsten,
Joe Damato, Alexander Viro, Christian Brauner, Jan Kara,
open list:FILESYSTEMS (VFS and infrastructure), open list
From: Martin Karsten <mkarsten@uwaterloo.ca>
When events are reported to userland and prefer_busy_poll is set, irqs
are temporarily suspended using napi_suspend_irqs.
If no events are found and ep_poll would go to sleep, irq suspension is
cancelled using napi_resume_irqs.
Signed-off-by: Martin Karsten <mkarsten@uwaterloo.ca>
Co-developed-by: Joe Damato <jdamato@fastly.com>
Signed-off-by: Joe Damato <jdamato@fastly.com>
Tested-by: Joe Damato <jdamato@fastly.com>
Tested-by: Martin Karsten <mkarsten@uwaterloo.ca>
Acked-by: Stanislav Fomichev <sdf@fomichev.me>
Reviewed-by: Sridhar Samudrala <sridhar.samudrala@intel.com>
---
v5:
- Only call ep_suspend_napi_irqs when ep_send_events returns a
positive value. IRQs are not suspended in error (e.g. EINTR)
cases. This issue was pointed out by Hillf Danton.
rfc -> v1:
- move irq resume code from ep_free to a helper which either resumes
IRQs or does nothing if !defined(CONFIG_NET_RX_BUSY_POLL).
fs/eventpoll.c | 32 +++++++++++++++++++++++++++++++-
1 file changed, 31 insertions(+), 1 deletion(-)
diff --git a/fs/eventpoll.c b/fs/eventpoll.c
index f9e0d9307dad..83bcb559b89f 100644
--- a/fs/eventpoll.c
+++ b/fs/eventpoll.c
@@ -457,6 +457,8 @@ static bool ep_busy_loop(struct eventpoll *ep, int nonblock)
* it back in when we have moved a socket with a valid NAPI
* ID onto the ready list.
*/
+ if (prefer_busy_poll)
+ napi_resume_irqs(napi_id);
ep->napi_id = 0;
return false;
}
@@ -540,6 +542,22 @@ static long ep_eventpoll_bp_ioctl(struct file *file, unsigned int cmd,
}
}
+static void ep_suspend_napi_irqs(struct eventpoll *ep)
+{
+ unsigned int napi_id = READ_ONCE(ep->napi_id);
+
+ if (napi_id >= MIN_NAPI_ID && READ_ONCE(ep->prefer_busy_poll))
+ napi_suspend_irqs(napi_id);
+}
+
+static void ep_resume_napi_irqs(struct eventpoll *ep)
+{
+ unsigned int napi_id = READ_ONCE(ep->napi_id);
+
+ if (napi_id >= MIN_NAPI_ID && READ_ONCE(ep->prefer_busy_poll))
+ napi_resume_irqs(napi_id);
+}
+
#else
static inline bool ep_busy_loop(struct eventpoll *ep, int nonblock)
@@ -557,6 +575,14 @@ static long ep_eventpoll_bp_ioctl(struct file *file, unsigned int cmd,
return -EOPNOTSUPP;
}
+static void ep_suspend_napi_irqs(struct eventpoll *ep)
+{
+}
+
+static void ep_resume_napi_irqs(struct eventpoll *ep)
+{
+}
+
#endif /* CONFIG_NET_RX_BUSY_POLL */
/*
@@ -788,6 +814,7 @@ static bool ep_refcount_dec_and_test(struct eventpoll *ep)
static void ep_free(struct eventpoll *ep)
{
+ ep_resume_napi_irqs(ep);
mutex_destroy(&ep->mtx);
free_uid(ep->user);
wakeup_source_unregister(ep->ws);
@@ -2005,8 +2032,11 @@ static int ep_poll(struct eventpoll *ep, struct epoll_event __user *events,
* trying again in search of more luck.
*/
res = ep_send_events(ep, events, maxevents);
- if (res)
+ if (res) {
+ if (res > 0)
+ ep_suspend_napi_irqs(ep);
return res;
+ }
}
if (timed_out)
--
2.25.1
^ permalink raw reply related [flat|nested] 17+ messages in thread* [PATCH net-next v6 6/7] selftests: net: Add busy_poll_test
2024-11-04 21:55 [PATCH net-next v6 0/7] Suspend IRQs during application busy periods Joe Damato
` (4 preceding siblings ...)
2024-11-04 21:55 ` [PATCH net-next v6 5/7] eventpoll: Control irq suspension for prefer_busy_poll Joe Damato
@ 2024-11-04 21:55 ` Joe Damato
2024-11-05 3:50 ` Stanislav Fomichev
2024-11-04 21:55 ` [PATCH net-next v6 7/7] docs: networking: Describe irq suspension Joe Damato
6 siblings, 1 reply; 17+ messages in thread
From: Joe Damato @ 2024-11-04 21:55 UTC (permalink / raw)
To: netdev
Cc: corbet, hdanton, bagasdotme, pabeni, namangulati, edumazet,
amritha.nambiar, sridhar.samudrala, sdf, peter, m2shafiei, bjorn,
hch, willy, willemdebruijn.kernel, skhawaja, kuba, Joe Damato,
Martin Karsten, David S. Miller, Simon Horman, Shuah Khan,
open list, open list:KERNEL SELFTEST FRAMEWORK
Add an epoll busy poll test using netdevsim.
This test is comprised of:
- busy_poller (via busy_poller.c)
- busy_poll_test.sh which loads netdevsim, sets up network namespaces,
and runs busy_poller to receive data and socat to send data.
The selftest tests two different scenarios:
- busy poll (the pre-existing version in the kernel)
- busy poll with suspend enabled (what this series adds)
The data transmit is a 1MiB temporary file generated from /dev/urandom
and the test is considered passing if the md5sum of the input file to
socat matches the md5sum of the output file from busy_poller.
netdevsim was chosen instead of veth due to netdevsim's support for
netdev-genl.
For now, this test uses the functionality that netdevsim provides. In the
future, perhaps netdevsim can be extended to emulate device IRQs to more
thoroughly test all pre-existing kernel options (like defer_hard_irqs)
and suspend.
Signed-off-by: Joe Damato <jdamato@fastly.com>
Co-developed-by: Martin Karsten <mkarsten@uwaterloo.ca>
Signed-off-by: Martin Karsten <mkarsten@uwaterloo.ca>
---
v5:
- Updated commit message to replace netcat with socat and fixed
misspelling of netdevsim. No functional/code changes.
v4:
- Updated busy_poll_test.sh:
- use socat instead of nc
- drop cli.py usage from the script
- removed check_ynl
- Updated busy_poller.c:
- use netlink to configure napi parameters
v3:
- New in v3
tools/testing/selftests/net/.gitignore | 1 +
tools/testing/selftests/net/Makefile | 3 +-
tools/testing/selftests/net/busy_poll_test.sh | 164 +++++++++
tools/testing/selftests/net/busy_poller.c | 328 ++++++++++++++++++
4 files changed, 495 insertions(+), 1 deletion(-)
create mode 100755 tools/testing/selftests/net/busy_poll_test.sh
create mode 100644 tools/testing/selftests/net/busy_poller.c
diff --git a/tools/testing/selftests/net/.gitignore b/tools/testing/selftests/net/.gitignore
index 217d8b7a7365..85b0c4a2179f 100644
--- a/tools/testing/selftests/net/.gitignore
+++ b/tools/testing/selftests/net/.gitignore
@@ -2,6 +2,7 @@
bind_bhash
bind_timewait
bind_wildcard
+busy_poller
cmsg_sender
diag_uid
epoll_busy_poll
diff --git a/tools/testing/selftests/net/Makefile b/tools/testing/selftests/net/Makefile
index 26a4883a65c9..3ccfe454db1a 100644
--- a/tools/testing/selftests/net/Makefile
+++ b/tools/testing/selftests/net/Makefile
@@ -96,9 +96,10 @@ TEST_PROGS += fdb_flush.sh
TEST_PROGS += fq_band_pktlimit.sh
TEST_PROGS += vlan_hw_filter.sh
TEST_PROGS += bpf_offload.py
+TEST_PROGS += busy_poll_test.sh
# YNL files, must be before "include ..lib.mk"
-YNL_GEN_FILES := ncdevmem
+YNL_GEN_FILES := ncdevmem busy_poller
TEST_GEN_FILES += $(YNL_GEN_FILES)
TEST_FILES := settings
diff --git a/tools/testing/selftests/net/busy_poll_test.sh b/tools/testing/selftests/net/busy_poll_test.sh
new file mode 100755
index 000000000000..ffc74bc62e5a
--- /dev/null
+++ b/tools/testing/selftests/net/busy_poll_test.sh
@@ -0,0 +1,164 @@
+#!/bin/bash
+# SPDX-License-Identifier: GPL-2.0-only
+source net_helper.sh
+
+NSIM_DEV_1_ID=$((256 + RANDOM % 256))
+NSIM_DEV_1_SYS=/sys/bus/netdevsim/devices/netdevsim$NSIM_DEV_1_ID
+NSIM_DEV_2_ID=$((512 + RANDOM % 256))
+NSIM_DEV_2_SYS=/sys/bus/netdevsim/devices/netdevsim$NSIM_DEV_2_ID
+
+NSIM_DEV_SYS_NEW=/sys/bus/netdevsim/new_device
+NSIM_DEV_SYS_DEL=/sys/bus/netdevsim/del_device
+NSIM_DEV_SYS_LINK=/sys/bus/netdevsim/link_device
+NSIM_DEV_SYS_UNLINK=/sys/bus/netdevsim/unlink_device
+
+setup_ns()
+{
+ set -e
+ ip netns add nssv
+ ip netns add nscl
+
+ NSIM_DEV_1_NAME=$(find $NSIM_DEV_1_SYS/net -maxdepth 1 -type d ! \
+ -path $NSIM_DEV_1_SYS/net -exec basename {} \;)
+ NSIM_DEV_2_NAME=$(find $NSIM_DEV_2_SYS/net -maxdepth 1 -type d ! \
+ -path $NSIM_DEV_2_SYS/net -exec basename {} \;)
+
+ # ensure the server has 1 queue
+ ethtool -L $NSIM_DEV_1_NAME combined 1 2>/dev/null
+
+ ip link set $NSIM_DEV_1_NAME netns nssv
+ ip link set $NSIM_DEV_2_NAME netns nscl
+
+ ip netns exec nssv ip addr add '192.168.1.1/24' dev $NSIM_DEV_1_NAME
+ ip netns exec nscl ip addr add '192.168.1.2/24' dev $NSIM_DEV_2_NAME
+
+ ip netns exec nssv ip link set dev $NSIM_DEV_1_NAME up
+ ip netns exec nscl ip link set dev $NSIM_DEV_2_NAME up
+
+ set +e
+}
+
+cleanup_ns()
+{
+ ip netns del nscl
+ ip netns del nssv
+}
+
+test_busypoll()
+{
+ tmp_file=$(mktemp)
+ out_file=$(mktemp)
+
+ # fill a test file with random data
+ dd if=/dev/urandom of=${tmp_file} bs=1M count=1 2> /dev/null
+
+ timeout -k 1s 30s ip netns exec nssv ./busy_poller -p48675 -b192.168.1.1 -m8 -u0 -P1 -g16 -i${NSIM_DEV_1_IFIDX} -o${out_file}&
+
+ wait_local_port_listen nssv 48675 tcp
+
+ ip netns exec nscl socat -u $tmp_file TCP:192.168.1.1:48675
+
+ wait
+
+ tmp_file_md5sum=$(md5sum $tmp_file | cut -f1 -d' ')
+ out_file_md5sum=$(md5sum $out_file | cut -f1 -d' ')
+
+ if [ "$tmp_file_md5sum" = "$out_file_md5sum" ]; then
+ res=0
+ else
+ echo "md5sum mismatch"
+ echo "input file md5sum: ${tmp_file_md5sum}";
+ echo "output file md5sum: ${out_file_md5sum}";
+ res=1
+ fi
+
+ rm $out_file $tmp_file
+
+ return $res
+}
+
+test_busypoll_with_suspend()
+{
+ tmp_file=$(mktemp)
+ out_file=$(mktemp)
+
+ # fill a test file with random data
+ dd if=/dev/urandom of=${tmp_file} bs=1M count=1 2> /dev/null
+
+ timeout -k 1s 30s ip netns exec nssv ./busy_poller -p48675 -b192.168.1.1 -m8 -u0 -P1 -g16 -d100 -r50000 -s20000000 -i${NSIM_DEV_1_IFIDX} -o${out_file}&
+
+ wait_local_port_listen nssv 48675 tcp
+
+ ip netns exec nscl socat -u $tmp_file TCP:192.168.1.1:48675
+
+ wait
+
+ tmp_file_md5sum=$(md5sum $tmp_file | cut -f1 -d' ')
+ out_file_md5sum=$(md5sum $out_file | cut -f1 -d' ')
+
+ if [ "$tmp_file_md5sum" = "$out_file_md5sum" ]; then
+ res=0
+ else
+ echo "md5sum mismatch"
+ echo "input file md5sum: ${tmp_file_md5sum}";
+ echo "output file md5sum: ${out_file_md5sum}";
+ res=1
+ fi
+
+ rm $out_file $tmp_file
+
+ return $res
+}
+
+###
+### Code start
+###
+
+modprobe netdevsim
+
+# linking
+
+echo $NSIM_DEV_1_ID > $NSIM_DEV_SYS_NEW
+echo $NSIM_DEV_2_ID > $NSIM_DEV_SYS_NEW
+udevadm settle
+
+setup_ns
+
+NSIM_DEV_1_FD=$((256 + RANDOM % 256))
+exec {NSIM_DEV_1_FD}</var/run/netns/nssv
+NSIM_DEV_1_IFIDX=$(ip netns exec nssv cat /sys/class/net/$NSIM_DEV_1_NAME/ifindex)
+
+NSIM_DEV_2_FD=$((256 + RANDOM % 256))
+exec {NSIM_DEV_2_FD}</var/run/netns/nscl
+NSIM_DEV_2_IFIDX=$(ip netns exec nscl cat /sys/class/net/$NSIM_DEV_2_NAME/ifindex)
+
+echo "$NSIM_DEV_1_FD:$NSIM_DEV_1_IFIDX $NSIM_DEV_2_FD:$NSIM_DEV_2_IFIDX" > $NSIM_DEV_SYS_LINK
+if [ $? -ne 0 ]; then
+ echo "linking netdevsim1 with netdevsim2 should succeed"
+ cleanup_ns
+ exit 1
+fi
+
+test_busypoll
+if [ $? -ne 0 ]; then
+ echo "test_busypoll failed"
+ cleanup_ns
+ exit 1
+fi
+
+test_busypoll_with_suspend
+if [ $? -ne 0 ]; then
+ echo "test_busypoll_with_suspend failed"
+ cleanup_ns
+ exit 1
+fi
+
+echo "$NSIM_DEV_1_FD:$NSIM_DEV_1_IFIDX" > $NSIM_DEV_SYS_UNLINK
+
+echo $NSIM_DEV_2_ID > $NSIM_DEV_SYS_DEL
+
+cleanup_ns
+
+modprobe -r netdevsim
+
+exit 0
diff --git a/tools/testing/selftests/net/busy_poller.c b/tools/testing/selftests/net/busy_poller.c
new file mode 100644
index 000000000000..8d8aa9e5939a
--- /dev/null
+++ b/tools/testing/selftests/net/busy_poller.c
@@ -0,0 +1,328 @@
+// SPDX-License-Identifier: GPL-2.0
+#include <assert.h>
+#include <errno.h>
+#include <error.h>
+#include <fcntl.h>
+#include <inttypes.h>
+#include <limits.h>
+#include <stdlib.h>
+#include <stdio.h>
+#include <string.h>
+#include <unistd.h>
+
+#include <arpa/inet.h>
+#include <netinet/in.h>
+
+#include <sys/ioctl.h>
+#include <sys/epoll.h>
+#include <sys/socket.h>
+#include <sys/types.h>
+
+#include <linux/netlink.h>
+#include <linux/genetlink.h>
+#include "netdev-user.h"
+#include <ynl.h>
+
+/* if the headers haven't been updated, we need to define some things */
+#if !defined(EPOLL_IOC_TYPE)
+struct epoll_params {
+ uint32_t busy_poll_usecs;
+ uint16_t busy_poll_budget;
+ uint8_t prefer_busy_poll;
+
+ /* pad the struct to a multiple of 64bits */
+ uint8_t __pad;
+};
+
+#define EPOLL_IOC_TYPE 0x8A
+#define EPIOCSPARAMS _IOW(EPOLL_IOC_TYPE, 0x01, struct epoll_params)
+#define EPIOCGPARAMS _IOR(EPOLL_IOC_TYPE, 0x02, struct epoll_params)
+#endif
+
+static uint32_t cfg_port = 8000;
+static struct in_addr cfg_bind_addr = { .s_addr = INADDR_ANY };
+static char *cfg_outfile;
+static int cfg_max_events = 8;
+static int cfg_ifindex;
+
+/* busy poll params */
+static uint32_t cfg_busy_poll_usecs;
+static uint16_t cfg_busy_poll_budget;
+static uint8_t cfg_prefer_busy_poll;
+
+/* IRQ params */
+static uint32_t cfg_defer_hard_irqs;
+static uint64_t cfg_gro_flush_timeout;
+static uint64_t cfg_irq_suspend_timeout;
+
+static void usage(const char *filepath)
+{
+ error(1, 0,
+ "Usage: %s -p<port> -b<addr> -m<max_events> -u<busy_poll_usecs> -P<prefer_busy_poll> -g<busy_poll_budget> -o<outfile> -d<defer_hard_irqs> -r<gro_flush_timeout> -s<irq_suspend_timeout> -i<ifindex>",
+ filepath);
+}
+
+static void parse_opts(int argc, char **argv)
+{
+ int ret;
+ int c;
+
+ if (argc <= 1)
+ usage(argv[0]);
+
+ while ((c = getopt(argc, argv, "p:m:b:u:P:g:o:d:r:s:i:")) != -1) {
+ switch (c) {
+ case 'u':
+ cfg_busy_poll_usecs = strtoul(optarg, NULL, 0);
+ if (cfg_busy_poll_usecs == ULONG_MAX ||
+ cfg_busy_poll_usecs > UINT32_MAX)
+ error(1, ERANGE, "busy_poll_usecs too large");
+ break;
+ case 'P':
+ cfg_prefer_busy_poll = strtoul(optarg, NULL, 0);
+ if (cfg_prefer_busy_poll == ULONG_MAX ||
+ cfg_prefer_busy_poll > 1)
+ error(1, ERANGE,
+ "prefer busy poll should be 0 or 1");
+ break;
+ case 'g':
+ cfg_busy_poll_budget = strtoul(optarg, NULL, 0);
+ if (cfg_busy_poll_budget == ULONG_MAX ||
+ cfg_busy_poll_budget > UINT16_MAX)
+ error(1, ERANGE,
+ "busy poll budget must be [0, UINT16_MAX]");
+ break;
+ case 'p':
+ cfg_port = strtoul(optarg, NULL, 0);
+ if (cfg_port > UINT16_MAX)
+ error(1, ERANGE, "port must be <= 65535");
+ break;
+ case 'b':
+ ret = inet_aton(optarg, &cfg_bind_addr);
+ if (ret == 0)
+ error(1, errno,
+ "bind address %s invalid", optarg);
+ break;
+ case 'o':
+ cfg_outfile = strdup(optarg);
+ if (!cfg_outfile)
+ error(1, 0, "outfile invalid");
+ break;
+ case 'm':
+ cfg_max_events = strtol(optarg, NULL, 0);
+
+ if (cfg_max_events == LONG_MIN ||
+ cfg_max_events == LONG_MAX ||
+ cfg_max_events <= 0)
+ error(1, ERANGE,
+ "max events must be > 0 and < LONG_MAX");
+ break;
+ case 'd':
+ cfg_defer_hard_irqs = strtoul(optarg, NULL, 0);
+
+ if (cfg_defer_hard_irqs == ULONG_MAX ||
+ cfg_defer_hard_irqs > INT32_MAX)
+ error(1, ERANGE,
+ "defer_hard_irqs must be <= INT32_MAX");
+ break;
+ case 'r':
+ cfg_gro_flush_timeout = strtoull(optarg, NULL, 0);
+
+ if (cfg_gro_flush_timeout == ULLONG_MAX)
+ error(1, ERANGE,
+ "gro_flush_timeout must be < ULLONG_MAX");
+ break;
+ case 's':
+ cfg_irq_suspend_timeout = strtoull(optarg, NULL, 0);
+
+ if (cfg_irq_suspend_timeout == ULLONG_MAX)
+ error(1, ERANGE,
+ "irq_suspend_timeout must be < ULLONG_MAX");
+ break;
+ case 'i':
+ cfg_ifindex = strtoul(optarg, NULL, 0);
+ if (cfg_ifindex == ULONG_MAX)
+ error(1, ERANGE,
+ "ifindex must be < ULONG_MAX");
+ break;
+ }
+ }
+
+ if (!cfg_ifindex)
+ usage(argv[0]);
+
+ if (optind != argc)
+ usage(argv[0]);
+}
+
+static void epoll_ctl_add(int epfd, int fd, uint32_t events)
+{
+ struct epoll_event ev;
+
+ ev.events = events;
+ ev.data.fd = fd;
+ if (epoll_ctl(epfd, EPOLL_CTL_ADD, fd, &ev) == -1)
+ error(1, errno, "epoll_ctl add fd: %d", fd);
+}
+
+static void setnonblock(int sockfd)
+{
+ int flags;
+
+ flags = fcntl(sockfd, F_GETFL, 0);
+
+ if (fcntl(sockfd, F_SETFL, flags | O_NONBLOCK) == -1)
+ error(1, errno, "unable to set socket to nonblocking mode");
+}
+
+static void write_chunk(int fd, char *buf, ssize_t buflen)
+{
+ ssize_t remaining = buflen;
+ char *buf_offset = buf;
+ ssize_t writelen = 0;
+ ssize_t write_result;
+
+ while (writelen < buflen) {
+ write_result = write(fd, buf_offset, remaining);
+ if (write_result == -1)
+ error(1, errno, "unable to write data to outfile");
+
+ writelen += write_result;
+ remaining -= write_result;
+ buf_offset += write_result;
+ }
+}
+
+static void setup_queue(void)
+{
+ struct netdev_napi_get_list *napi_list = NULL;
+ struct netdev_napi_get_req_dump *req = NULL;
+ struct netdev_napi_set_req *set_req = NULL;
+ struct ynl_sock *ys;
+ struct ynl_error yerr;
+ uint32_t napi_id;
+
+ ys = ynl_sock_create(&ynl_netdev_family, &yerr);
+ if (!ys)
+ error(1, 0, "YNL: %s", yerr.msg);
+
+ req = netdev_napi_get_req_dump_alloc();
+ netdev_napi_get_req_dump_set_ifindex(req, cfg_ifindex);
+ napi_list = netdev_napi_get_dump(ys, req);
+
+ /* assume there is 1 NAPI configured and take the first */
+ if (napi_list->obj._present.id)
+ napi_id = napi_list->obj.id;
+ else
+ error(1, 0, "napi ID not present?");
+
+ set_req = netdev_napi_set_req_alloc();
+ netdev_napi_set_req_set_id(set_req, napi_id);
+ netdev_napi_set_req_set_defer_hard_irqs(set_req, cfg_defer_hard_irqs);
+ netdev_napi_set_req_set_gro_flush_timeout(set_req,
+ cfg_gro_flush_timeout);
+ netdev_napi_set_req_set_irq_suspend_timeout(set_req,
+ cfg_irq_suspend_timeout);
+
+ if (netdev_napi_set(ys, set_req))
+ error(1, 0, "can't set NAPI params: %s\n", yerr.msg);
+
+ netdev_napi_get_list_free(napi_list);
+ netdev_napi_get_req_dump_free(req);
+ netdev_napi_set_req_free(set_req);
+ ynl_sock_destroy(ys);
+}
+
+static void run_poller(void)
+{
+ struct epoll_event events[cfg_max_events];
+ struct epoll_params epoll_params = {0};
+ struct sockaddr_in server_addr;
+ int i, epfd, nfds;
+ ssize_t readlen;
+ int outfile_fd;
+ char buf[1024];
+ int sockfd;
+ int conn;
+ int val;
+
+ outfile_fd = open(cfg_outfile, O_WRONLY | O_CREAT, 0644);
+ if (outfile_fd == -1)
+ error(1, errno, "unable to open outfile: %s", cfg_outfile);
+
+ sockfd = socket(AF_INET, SOCK_STREAM, IPPROTO_TCP);
+ if (sockfd == -1)
+ error(1, errno, "unable to create listen socket");
+
+ server_addr.sin_family = AF_INET;
+ server_addr.sin_port = htons(cfg_port);
+ server_addr.sin_addr = cfg_bind_addr;
+
+ epoll_params.busy_poll_usecs = cfg_busy_poll_usecs;
+ epoll_params.busy_poll_budget = cfg_busy_poll_budget;
+ epoll_params.prefer_busy_poll = cfg_prefer_busy_poll;
+ epoll_params.__pad = 0;
+
+ val = 1;
+ if (setsockopt(sockfd, SOL_SOCKET, SO_REUSEADDR, &val, sizeof(val)))
+ error(1, errno, "poller setsockopt reuseaddr");
+
+ setnonblock(sockfd);
+
+ if (bind(sockfd, (struct sockaddr *)&server_addr,
+ sizeof(struct sockaddr_in)))
+ error(0, errno, "poller bind to port: %d\n", cfg_port);
+
+ if (listen(sockfd, 1))
+ error(1, errno, "poller listen");
+
+ epfd = epoll_create1(0);
+ if (ioctl(epfd, EPIOCSPARAMS, &epoll_params) == -1)
+ error(1, errno, "unable to set busy poll params");
+
+ epoll_ctl_add(epfd, sockfd, EPOLLIN | EPOLLOUT | EPOLLET);
+
+ for (;;) {
+ nfds = epoll_wait(epfd, events, cfg_max_events, -1);
+ for (i = 0; i < nfds; i++) {
+ if (events[i].data.fd == sockfd) {
+ conn = accept(sockfd, NULL, NULL);
+ if (conn == -1)
+ error(1, errno,
+ "accepting incoming connection failed");
+
+ setnonblock(conn);
+ epoll_ctl_add(epfd, conn,
+ EPOLLIN | EPOLLET | EPOLLRDHUP |
+ EPOLLHUP);
+ } else if (events[i].events & EPOLLIN) {
+ for (;;) {
+ readlen = read(events[i].data.fd, buf,
+ sizeof(buf));
+ if (readlen > 0)
+ write_chunk(outfile_fd, buf,
+ readlen);
+ else
+ break;
+ }
+ } else {
+ /* spurious event ? */
+ }
+ if (events[i].events & (EPOLLRDHUP | EPOLLHUP)) {
+ epoll_ctl(epfd, EPOLL_CTL_DEL,
+ events[i].data.fd, NULL);
+ close(events[i].data.fd);
+ close(outfile_fd);
+ return;
+ }
+ }
+ }
+}
+
+int main(int argc, char *argv[])
+{
+ parse_opts(argc, argv);
+ setup_queue();
+ run_poller();
+ return 0;
+}
--
2.25.1
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [PATCH net-next v6 6/7] selftests: net: Add busy_poll_test
2024-11-04 21:55 ` [PATCH net-next v6 6/7] selftests: net: Add busy_poll_test Joe Damato
@ 2024-11-05 3:50 ` Stanislav Fomichev
2024-11-05 17:49 ` Joe Damato
0 siblings, 1 reply; 17+ messages in thread
From: Stanislav Fomichev @ 2024-11-05 3:50 UTC (permalink / raw)
To: Joe Damato
Cc: netdev, corbet, hdanton, bagasdotme, pabeni, namangulati,
edumazet, amritha.nambiar, sridhar.samudrala, sdf, peter,
m2shafiei, bjorn, hch, willy, willemdebruijn.kernel, skhawaja,
kuba, Martin Karsten, David S. Miller, Simon Horman, Shuah Khan,
open list, open list:KERNEL SELFTEST FRAMEWORK
On 11/04, Joe Damato wrote:
> Add an epoll busy poll test using netdevsim.
>
> This test is comprised of:
> - busy_poller (via busy_poller.c)
> - busy_poll_test.sh which loads netdevsim, sets up network namespaces,
> and runs busy_poller to receive data and socat to send data.
>
> The selftest tests two different scenarios:
> - busy poll (the pre-existing version in the kernel)
> - busy poll with suspend enabled (what this series adds)
>
> The data transmit is a 1MiB temporary file generated from /dev/urandom
> and the test is considered passing if the md5sum of the input file to
> socat matches the md5sum of the output file from busy_poller.
>
> netdevsim was chosen instead of veth due to netdevsim's support for
> netdev-genl.
>
> For now, this test uses the functionality that netdevsim provides. In the
> future, perhaps netdevsim can be extended to emulate device IRQs to more
> thoroughly test all pre-existing kernel options (like defer_hard_irqs)
> and suspend.
>
> Signed-off-by: Joe Damato <jdamato@fastly.com>
> Co-developed-by: Martin Karsten <mkarsten@uwaterloo.ca>
> Signed-off-by: Martin Karsten <mkarsten@uwaterloo.ca>
> ---
> v5:
> - Updated commit message to replace netcat with socat and fixed
> misspelling of netdevsim. No functional/code changes.
>
> v4:
> - Updated busy_poll_test.sh:
> - use socat instead of nc
> - drop cli.py usage from the script
> - removed check_ynl
> - Updated busy_poller.c:
> - use netlink to configure napi parameters
>
> v3:
> - New in v3
>
> tools/testing/selftests/net/.gitignore | 1 +
> tools/testing/selftests/net/Makefile | 3 +-
> tools/testing/selftests/net/busy_poll_test.sh | 164 +++++++++
> tools/testing/selftests/net/busy_poller.c | 328 ++++++++++++++++++
> 4 files changed, 495 insertions(+), 1 deletion(-)
> create mode 100755 tools/testing/selftests/net/busy_poll_test.sh
> create mode 100644 tools/testing/selftests/net/busy_poller.c
>
> diff --git a/tools/testing/selftests/net/.gitignore b/tools/testing/selftests/net/.gitignore
> index 217d8b7a7365..85b0c4a2179f 100644
> --- a/tools/testing/selftests/net/.gitignore
> +++ b/tools/testing/selftests/net/.gitignore
> @@ -2,6 +2,7 @@
> bind_bhash
> bind_timewait
> bind_wildcard
> +busy_poller
> cmsg_sender
> diag_uid
> epoll_busy_poll
> diff --git a/tools/testing/selftests/net/Makefile b/tools/testing/selftests/net/Makefile
> index 26a4883a65c9..3ccfe454db1a 100644
> --- a/tools/testing/selftests/net/Makefile
> +++ b/tools/testing/selftests/net/Makefile
> @@ -96,9 +96,10 @@ TEST_PROGS += fdb_flush.sh
> TEST_PROGS += fq_band_pktlimit.sh
> TEST_PROGS += vlan_hw_filter.sh
> TEST_PROGS += bpf_offload.py
> +TEST_PROGS += busy_poll_test.sh
>
> # YNL files, must be before "include ..lib.mk"
> -YNL_GEN_FILES := ncdevmem
> +YNL_GEN_FILES := ncdevmem busy_poller
> TEST_GEN_FILES += $(YNL_GEN_FILES)
>
> TEST_FILES := settings
> diff --git a/tools/testing/selftests/net/busy_poll_test.sh b/tools/testing/selftests/net/busy_poll_test.sh
> new file mode 100755
> index 000000000000..ffc74bc62e5a
> --- /dev/null
> +++ b/tools/testing/selftests/net/busy_poll_test.sh
> @@ -0,0 +1,164 @@
> +#!/bin/bash
> +# SPDX-License-Identifier: GPL-2.0-only
> +source net_helper.sh
> +
> +NSIM_DEV_1_ID=$((256 + RANDOM % 256))
> +NSIM_DEV_1_SYS=/sys/bus/netdevsim/devices/netdevsim$NSIM_DEV_1_ID
> +NSIM_DEV_2_ID=$((512 + RANDOM % 256))
> +NSIM_DEV_2_SYS=/sys/bus/netdevsim/devices/netdevsim$NSIM_DEV_2_ID
> +
> +NSIM_DEV_SYS_NEW=/sys/bus/netdevsim/new_device
> +NSIM_DEV_SYS_DEL=/sys/bus/netdevsim/del_device
> +NSIM_DEV_SYS_LINK=/sys/bus/netdevsim/link_device
> +NSIM_DEV_SYS_UNLINK=/sys/bus/netdevsim/unlink_device
> +
> +setup_ns()
> +{
> + set -e
> + ip netns add nssv
> + ip netns add nscl
> +
> + NSIM_DEV_1_NAME=$(find $NSIM_DEV_1_SYS/net -maxdepth 1 -type d ! \
> + -path $NSIM_DEV_1_SYS/net -exec basename {} \;)
> + NSIM_DEV_2_NAME=$(find $NSIM_DEV_2_SYS/net -maxdepth 1 -type d ! \
> + -path $NSIM_DEV_2_SYS/net -exec basename {} \;)
> +
> + # ensure the server has 1 queue
> + ethtool -L $NSIM_DEV_1_NAME combined 1 2>/dev/null
> +
> + ip link set $NSIM_DEV_1_NAME netns nssv
> + ip link set $NSIM_DEV_2_NAME netns nscl
> +
> + ip netns exec nssv ip addr add '192.168.1.1/24' dev $NSIM_DEV_1_NAME
> + ip netns exec nscl ip addr add '192.168.1.2/24' dev $NSIM_DEV_2_NAME
> +
> + ip netns exec nssv ip link set dev $NSIM_DEV_1_NAME up
> + ip netns exec nscl ip link set dev $NSIM_DEV_2_NAME up
> +
> + set +e
> +}
> +
> +cleanup_ns()
> +{
> + ip netns del nscl
> + ip netns del nssv
> +}
> +
> +test_busypoll()
> +{
> + tmp_file=$(mktemp)
> + out_file=$(mktemp)
> +
> + # fill a test file with random data
> + dd if=/dev/urandom of=${tmp_file} bs=1M count=1 2> /dev/null
> +
> + timeout -k 1s 30s ip netns exec nssv ./busy_poller -p48675 -b192.168.1.1 -m8 -u0 -P1 -g16 -i${NSIM_DEV_1_IFIDX} -o${out_file}&
> +
> + wait_local_port_listen nssv 48675 tcp
> +
> + ip netns exec nscl socat -u $tmp_file TCP:192.168.1.1:48675
> +
> + wait
> +
> + tmp_file_md5sum=$(md5sum $tmp_file | cut -f1 -d' ')
> + out_file_md5sum=$(md5sum $out_file | cut -f1 -d' ')
> +
> + if [ "$tmp_file_md5sum" = "$out_file_md5sum" ]; then
> + res=0
> + else
> + echo "md5sum mismatch"
> + echo "input file md5sum: ${tmp_file_md5sum}";
> + echo "output file md5sum: ${out_file_md5sum}";
> + res=1
> + fi
> +
> + rm $out_file $tmp_file
> +
> + return $res
> +}
> +
> +test_busypoll_with_suspend()
> +{
> + tmp_file=$(mktemp)
> + out_file=$(mktemp)
> +
> + # fill a test file with random data
> + dd if=/dev/urandom of=${tmp_file} bs=1M count=1 2> /dev/null
> +
> + timeout -k 1s 30s ip netns exec nssv ./busy_poller -p48675 -b192.168.1.1 -m8 -u0 -P1 -g16 -d100 -r50000 -s20000000 -i${NSIM_DEV_1_IFIDX} -o${out_file}&
> +
> + wait_local_port_listen nssv 48675 tcp
> +
> + ip netns exec nscl socat -u $tmp_file TCP:192.168.1.1:48675
> +
> + wait
> +
> + tmp_file_md5sum=$(md5sum $tmp_file | cut -f1 -d' ')
> + out_file_md5sum=$(md5sum $out_file | cut -f1 -d' ')
> +
> + if [ "$tmp_file_md5sum" = "$out_file_md5sum" ]; then
> + res=0
> + else
> + echo "md5sum mismatch"
> + echo "input file md5sum: ${tmp_file_md5sum}";
> + echo "output file md5sum: ${out_file_md5sum}";
> + res=1
> + fi
> +
> + rm $out_file $tmp_file
> +
> + return $res
> +}
> +
> +###
> +### Code start
> +###
> +
> +modprobe netdevsim
> +
> +# linking
> +
> +echo $NSIM_DEV_1_ID > $NSIM_DEV_SYS_NEW
> +echo $NSIM_DEV_2_ID > $NSIM_DEV_SYS_NEW
> +udevadm settle
> +
> +setup_ns
> +
> +NSIM_DEV_1_FD=$((256 + RANDOM % 256))
> +exec {NSIM_DEV_1_FD}</var/run/netns/nssv
> +NSIM_DEV_1_IFIDX=$(ip netns exec nssv cat /sys/class/net/$NSIM_DEV_1_NAME/ifindex)
> +
> +NSIM_DEV_2_FD=$((256 + RANDOM % 256))
> +exec {NSIM_DEV_2_FD}</var/run/netns/nscl
> +NSIM_DEV_2_IFIDX=$(ip netns exec nscl cat /sys/class/net/$NSIM_DEV_2_NAME/ifindex)
> +
> +echo "$NSIM_DEV_1_FD:$NSIM_DEV_1_IFIDX $NSIM_DEV_2_FD:$NSIM_DEV_2_IFIDX" > $NSIM_DEV_SYS_LINK
> +if [ $? -ne 0 ]; then
> + echo "linking netdevsim1 with netdevsim2 should succeed"
> + cleanup_ns
> + exit 1
> +fi
> +
> +test_busypoll
> +if [ $? -ne 0 ]; then
> + echo "test_busypoll failed"
> + cleanup_ns
> + exit 1
> +fi
> +
> +test_busypoll_with_suspend
> +if [ $? -ne 0 ]; then
> + echo "test_busypoll_with_suspend failed"
> + cleanup_ns
> + exit 1
> +fi
> +
> +echo "$NSIM_DEV_1_FD:$NSIM_DEV_1_IFIDX" > $NSIM_DEV_SYS_UNLINK
> +
> +echo $NSIM_DEV_2_ID > $NSIM_DEV_SYS_DEL
> +
> +cleanup_ns
> +
> +modprobe -r netdevsim
> +
> +exit 0
> diff --git a/tools/testing/selftests/net/busy_poller.c b/tools/testing/selftests/net/busy_poller.c
> new file mode 100644
> index 000000000000..8d8aa9e5939a
> --- /dev/null
> +++ b/tools/testing/selftests/net/busy_poller.c
> @@ -0,0 +1,328 @@
> +// SPDX-License-Identifier: GPL-2.0
> +#include <assert.h>
> +#include <errno.h>
> +#include <error.h>
> +#include <fcntl.h>
> +#include <inttypes.h>
> +#include <limits.h>
> +#include <stdlib.h>
> +#include <stdio.h>
> +#include <string.h>
> +#include <unistd.h>
> +
> +#include <arpa/inet.h>
> +#include <netinet/in.h>
> +
> +#include <sys/ioctl.h>
> +#include <sys/epoll.h>
> +#include <sys/socket.h>
> +#include <sys/types.h>
> +
> +#include <linux/netlink.h>
> +#include <linux/genetlink.h>
> +#include "netdev-user.h"
> +#include <ynl.h>
> +
> +/* if the headers haven't been updated, we need to define some things */
> +#if !defined(EPOLL_IOC_TYPE)
> +struct epoll_params {
> + uint32_t busy_poll_usecs;
> + uint16_t busy_poll_budget;
> + uint8_t prefer_busy_poll;
> +
> + /* pad the struct to a multiple of 64bits */
> + uint8_t __pad;
> +};
> +
> +#define EPOLL_IOC_TYPE 0x8A
> +#define EPIOCSPARAMS _IOW(EPOLL_IOC_TYPE, 0x01, struct epoll_params)
> +#define EPIOCGPARAMS _IOR(EPOLL_IOC_TYPE, 0x02, struct epoll_params)
> +#endif
> +
> +static uint32_t cfg_port = 8000;
> +static struct in_addr cfg_bind_addr = { .s_addr = INADDR_ANY };
> +static char *cfg_outfile;
> +static int cfg_max_events = 8;
> +static int cfg_ifindex;
> +
> +/* busy poll params */
> +static uint32_t cfg_busy_poll_usecs;
> +static uint16_t cfg_busy_poll_budget;
> +static uint8_t cfg_prefer_busy_poll;
> +
> +/* IRQ params */
> +static uint32_t cfg_defer_hard_irqs;
> +static uint64_t cfg_gro_flush_timeout;
> +static uint64_t cfg_irq_suspend_timeout;
> +
> +static void usage(const char *filepath)
> +{
> + error(1, 0,
> + "Usage: %s -p<port> -b<addr> -m<max_events> -u<busy_poll_usecs> -P<prefer_busy_poll> -g<busy_poll_budget> -o<outfile> -d<defer_hard_irqs> -r<gro_flush_timeout> -s<irq_suspend_timeout> -i<ifindex>",
> + filepath);
> +}
> +
> +static void parse_opts(int argc, char **argv)
> +{
> + int ret;
> + int c;
> +
> + if (argc <= 1)
> + usage(argv[0]);
> +
> + while ((c = getopt(argc, argv, "p:m:b:u:P:g:o:d:r:s:i:")) != -1) {
> + switch (c) {
> + case 'u':
> + cfg_busy_poll_usecs = strtoul(optarg, NULL, 0);
> + if (cfg_busy_poll_usecs == ULONG_MAX ||
> + cfg_busy_poll_usecs > UINT32_MAX)
> + error(1, ERANGE, "busy_poll_usecs too large");
> + break;
> + case 'P':
> + cfg_prefer_busy_poll = strtoul(optarg, NULL, 0);
> + if (cfg_prefer_busy_poll == ULONG_MAX ||
> + cfg_prefer_busy_poll > 1)
> + error(1, ERANGE,
> + "prefer busy poll should be 0 or 1");
> + break;
> + case 'g':
> + cfg_busy_poll_budget = strtoul(optarg, NULL, 0);
> + if (cfg_busy_poll_budget == ULONG_MAX ||
> + cfg_busy_poll_budget > UINT16_MAX)
> + error(1, ERANGE,
> + "busy poll budget must be [0, UINT16_MAX]");
> + break;
> + case 'p':
> + cfg_port = strtoul(optarg, NULL, 0);
> + if (cfg_port > UINT16_MAX)
> + error(1, ERANGE, "port must be <= 65535");
> + break;
> + case 'b':
> + ret = inet_aton(optarg, &cfg_bind_addr);
> + if (ret == 0)
> + error(1, errno,
> + "bind address %s invalid", optarg);
> + break;
> + case 'o':
> + cfg_outfile = strdup(optarg);
> + if (!cfg_outfile)
> + error(1, 0, "outfile invalid");
> + break;
> + case 'm':
> + cfg_max_events = strtol(optarg, NULL, 0);
> +
> + if (cfg_max_events == LONG_MIN ||
> + cfg_max_events == LONG_MAX ||
> + cfg_max_events <= 0)
> + error(1, ERANGE,
> + "max events must be > 0 and < LONG_MAX");
> + break;
> + case 'd':
> + cfg_defer_hard_irqs = strtoul(optarg, NULL, 0);
> +
> + if (cfg_defer_hard_irqs == ULONG_MAX ||
> + cfg_defer_hard_irqs > INT32_MAX)
> + error(1, ERANGE,
> + "defer_hard_irqs must be <= INT32_MAX");
> + break;
> + case 'r':
> + cfg_gro_flush_timeout = strtoull(optarg, NULL, 0);
> +
> + if (cfg_gro_flush_timeout == ULLONG_MAX)
> + error(1, ERANGE,
> + "gro_flush_timeout must be < ULLONG_MAX");
> + break;
> + case 's':
> + cfg_irq_suspend_timeout = strtoull(optarg, NULL, 0);
> +
> + if (cfg_irq_suspend_timeout == ULLONG_MAX)
> + error(1, ERANGE,
> + "irq_suspend_timeout must be < ULLONG_MAX");
> + break;
> + case 'i':
> + cfg_ifindex = strtoul(optarg, NULL, 0);
> + if (cfg_ifindex == ULONG_MAX)
> + error(1, ERANGE,
> + "ifindex must be < ULONG_MAX");
> + break;
> + }
> + }
> +
> + if (!cfg_ifindex)
> + usage(argv[0]);
> +
> + if (optind != argc)
> + usage(argv[0]);
> +}
> +
> +static void epoll_ctl_add(int epfd, int fd, uint32_t events)
> +{
> + struct epoll_event ev;
> +
> + ev.events = events;
> + ev.data.fd = fd;
> + if (epoll_ctl(epfd, EPOLL_CTL_ADD, fd, &ev) == -1)
> + error(1, errno, "epoll_ctl add fd: %d", fd);
> +}
> +
> +static void setnonblock(int sockfd)
> +{
> + int flags;
> +
> + flags = fcntl(sockfd, F_GETFL, 0);
> +
> + if (fcntl(sockfd, F_SETFL, flags | O_NONBLOCK) == -1)
> + error(1, errno, "unable to set socket to nonblocking mode");
> +}
> +
> +static void write_chunk(int fd, char *buf, ssize_t buflen)
> +{
> + ssize_t remaining = buflen;
> + char *buf_offset = buf;
> + ssize_t writelen = 0;
> + ssize_t write_result;
> +
> + while (writelen < buflen) {
> + write_result = write(fd, buf_offset, remaining);
> + if (write_result == -1)
> + error(1, errno, "unable to write data to outfile");
> +
> + writelen += write_result;
> + remaining -= write_result;
> + buf_offset += write_result;
> + }
> +}
> +
> +static void setup_queue(void)
> +{
> + struct netdev_napi_get_list *napi_list = NULL;
> + struct netdev_napi_get_req_dump *req = NULL;
> + struct netdev_napi_set_req *set_req = NULL;
> + struct ynl_sock *ys;
> + struct ynl_error yerr;
> + uint32_t napi_id;
> +
> + ys = ynl_sock_create(&ynl_netdev_family, &yerr);
> + if (!ys)
> + error(1, 0, "YNL: %s", yerr.msg);
> +
> + req = netdev_napi_get_req_dump_alloc();
> + netdev_napi_get_req_dump_set_ifindex(req, cfg_ifindex);
> + napi_list = netdev_napi_get_dump(ys, req);
> +
> + /* assume there is 1 NAPI configured and take the first */
> + if (napi_list->obj._present.id)
> + napi_id = napi_list->obj.id;
> + else
> + error(1, 0, "napi ID not present?");
> +
> + set_req = netdev_napi_set_req_alloc();
> + netdev_napi_set_req_set_id(set_req, napi_id);
> + netdev_napi_set_req_set_defer_hard_irqs(set_req, cfg_defer_hard_irqs);
> + netdev_napi_set_req_set_gro_flush_timeout(set_req,
> + cfg_gro_flush_timeout);
> + netdev_napi_set_req_set_irq_suspend_timeout(set_req,
> + cfg_irq_suspend_timeout);
> +
> + if (netdev_napi_set(ys, set_req))
> + error(1, 0, "can't set NAPI params: %s\n", yerr.msg);
> +
> + netdev_napi_get_list_free(napi_list);
> + netdev_napi_get_req_dump_free(req);
> + netdev_napi_set_req_free(set_req);
> + ynl_sock_destroy(ys);
> +}
> +
> +static void run_poller(void)
> +{
> + struct epoll_event events[cfg_max_events];
> + struct epoll_params epoll_params = {0};
> + struct sockaddr_in server_addr;
> + int i, epfd, nfds;
> + ssize_t readlen;
> + int outfile_fd;
> + char buf[1024];
> + int sockfd;
> + int conn;
> + int val;
[..]
> + outfile_fd = open(cfg_outfile, O_WRONLY | O_CREAT, 0644);
> + if (outfile_fd == -1)
> + error(1, errno, "unable to open outfile: %s", cfg_outfile);
Any reason you're not printing to stdout? And then redirect it to a file
in the shell script if needed. Lets you save some code on open/close
and flag parsing :-p But I guess can keep it since you already have it
all working.
Acked-by: Stanislav Fomichev <sdf@fomichev.me>
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH net-next v6 6/7] selftests: net: Add busy_poll_test
2024-11-05 3:50 ` Stanislav Fomichev
@ 2024-11-05 17:49 ` Joe Damato
0 siblings, 0 replies; 17+ messages in thread
From: Joe Damato @ 2024-11-05 17:49 UTC (permalink / raw)
To: Stanislav Fomichev
Cc: netdev, corbet, hdanton, bagasdotme, pabeni, namangulati,
edumazet, amritha.nambiar, sridhar.samudrala, sdf, peter,
m2shafiei, bjorn, hch, willy, willemdebruijn.kernel, skhawaja,
kuba, Martin Karsten, David S. Miller, Simon Horman, Shuah Khan,
open list, open list:KERNEL SELFTEST FRAMEWORK
On Mon, Nov 04, 2024 at 07:50:21PM -0800, Stanislav Fomichev wrote:
> On 11/04, Joe Damato wrote:
> > Add an epoll busy poll test using netdevsim.
> >
> > This test is comprised of:
> > - busy_poller (via busy_poller.c)
> > - busy_poll_test.sh which loads netdevsim, sets up network namespaces,
> > and runs busy_poller to receive data and socat to send data.
> >
> > The selftest tests two different scenarios:
> > - busy poll (the pre-existing version in the kernel)
> > - busy poll with suspend enabled (what this series adds)
> >
> > The data transmit is a 1MiB temporary file generated from /dev/urandom
> > and the test is considered passing if the md5sum of the input file to
> > socat matches the md5sum of the output file from busy_poller.
> >
> > netdevsim was chosen instead of veth due to netdevsim's support for
> > netdev-genl.
> >
> > For now, this test uses the functionality that netdevsim provides. In the
> > future, perhaps netdevsim can be extended to emulate device IRQs to more
> > thoroughly test all pre-existing kernel options (like defer_hard_irqs)
> > and suspend.
> >
> > Signed-off-by: Joe Damato <jdamato@fastly.com>
> > Co-developed-by: Martin Karsten <mkarsten@uwaterloo.ca>
> > Signed-off-by: Martin Karsten <mkarsten@uwaterloo.ca>
> > ---
[...]
> > +
> > +static void run_poller(void)
> > +{
> > + struct epoll_event events[cfg_max_events];
> > + struct epoll_params epoll_params = {0};
> > + struct sockaddr_in server_addr;
> > + int i, epfd, nfds;
> > + ssize_t readlen;
> > + int outfile_fd;
> > + char buf[1024];
> > + int sockfd;
> > + int conn;
> > + int val;
>
> [..]
>
> > + outfile_fd = open(cfg_outfile, O_WRONLY | O_CREAT, 0644);
> > + if (outfile_fd == -1)
> > + error(1, errno, "unable to open outfile: %s", cfg_outfile);
>
> Any reason you're not printing to stdout? And then redirect it to a file
> in the shell script if needed. Lets you save some code on open/close
> and flag parsing :-p But I guess can keep it since you already have it
> all working.
No reason in particular; I thought about this while writing it, but
ended up adding it as a flag in case others come along to extend
this test in some capacity.
> Acked-by: Stanislav Fomichev <sdf@fomichev.me>
Thanks for the ack!
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH net-next v6 7/7] docs: networking: Describe irq suspension
2024-11-04 21:55 [PATCH net-next v6 0/7] Suspend IRQs during application busy periods Joe Damato
` (5 preceding siblings ...)
2024-11-04 21:55 ` [PATCH net-next v6 6/7] selftests: net: Add busy_poll_test Joe Damato
@ 2024-11-04 21:55 ` Joe Damato
2024-11-05 1:12 ` Bagas Sanjaya
6 siblings, 1 reply; 17+ messages in thread
From: Joe Damato @ 2024-11-04 21:55 UTC (permalink / raw)
To: netdev
Cc: corbet, hdanton, bagasdotme, pabeni, namangulati, edumazet,
amritha.nambiar, sridhar.samudrala, sdf, peter, m2shafiei, bjorn,
hch, willy, willemdebruijn.kernel, skhawaja, kuba, Joe Damato,
Martin Karsten, David S. Miller, Simon Horman,
open list:DOCUMENTATION, open list,
open list:BPF [MISC]:Keyword:(?:b|_)bpf(?:b|_)
Describe irq suspension, the epoll ioctls, and the tradeoffs of using
different gro_flush_timeout values.
Signed-off-by: Joe Damato <jdamato@fastly.com>
Co-developed-by: Martin Karsten <mkarsten@uwaterloo.ca>
Signed-off-by: Martin Karsten <mkarsten@uwaterloo.ca>
Reviewed-by: Sridhar Samudrala <sridhar.samudrala@intel.com>
---
v6:
- Fixed packet processing loop description based on feedback from
Bagas Sanjaya so that it renders properly when generated as html
v5:
- Fixed a minor typo in the epoll-based busy polling section
- Removed short paragraph referring to experimental data as that data
is not included in the documentation
v4:
- Updated documentation to further explain irq suspension
- Dropped Stanislav's Acked-by tag because of the doc changes
- Dropped Bagas' Reviewed-by tag because of the doc changes
v1 -> v2:
- Updated documentation to describe the per-NAPI configuration
parameters.
Documentation/networking/napi.rst | 170 +++++++++++++++++++++++++++++-
1 file changed, 168 insertions(+), 2 deletions(-)
diff --git a/Documentation/networking/napi.rst b/Documentation/networking/napi.rst
index dfa5d549be9c..02720dd71a76 100644
--- a/Documentation/networking/napi.rst
+++ b/Documentation/networking/napi.rst
@@ -192,6 +192,33 @@ is reused to control the delay of the timer, while
``napi_defer_hard_irqs`` controls the number of consecutive empty polls
before NAPI gives up and goes back to using hardware IRQs.
+The above parameters can also be set on a per-NAPI basis using netlink via
+netdev-genl. When used with netlink and configured on a per-NAPI basis, the
+parameters mentioned above use hyphens instead of underscores:
+``gro-flush-timeout`` and ``napi-defer-hard-irqs``.
+
+Per-NAPI configuration can be done programmatically in a user application
+or by using a script included in the kernel source tree:
+``tools/net/ynl/cli.py``.
+
+For example, using the script:
+
+.. code-block:: bash
+
+ $ kernel-source/tools/net/ynl/cli.py \
+ --spec Documentation/netlink/specs/netdev.yaml \
+ --do napi-set \
+ --json='{"id": 345,
+ "defer-hard-irqs": 111,
+ "gro-flush-timeout": 11111}'
+
+Similarly, the parameter ``irq-suspend-timeout`` can be set using netlink
+via netdev-genl. There is no global sysfs parameter for this value.
+
+``irq-suspend-timeout`` is used to determine how long an application can
+completely suspend IRQs. It is used in combination with SO_PREFER_BUSY_POLL,
+which can be set on a per-epoll context basis with ``EPIOCSPARAMS`` ioctl.
+
.. _poll:
Busy polling
@@ -207,6 +234,46 @@ selected sockets or using the global ``net.core.busy_poll`` and
``net.core.busy_read`` sysctls. An io_uring API for NAPI busy polling
also exists.
+epoll-based busy polling
+------------------------
+
+It is possible to trigger packet processing directly from calls to
+``epoll_wait``. In order to use this feature, a user application must ensure
+all file descriptors which are added to an epoll context have the same NAPI ID.
+
+If the application uses a dedicated acceptor thread, the application can obtain
+the NAPI ID of the incoming connection using SO_INCOMING_NAPI_ID and then
+distribute that file descriptor to a worker thread. The worker thread would add
+the file descriptor to its epoll context. This would ensure each worker thread
+has an epoll context with FDs that have the same NAPI ID.
+
+Alternatively, if the application uses SO_REUSEPORT, a bpf or ebpf program can
+be inserted to distribute incoming connections to threads such that each thread
+is only given incoming connections with the same NAPI ID. Care must be taken to
+carefully handle cases where a system may have multiple NICs.
+
+In order to enable busy polling, there are two choices:
+
+1. ``/proc/sys/net/core/busy_poll`` can be set with a time in useconds to busy
+ loop waiting for events. This is a system-wide setting and will cause all
+ epoll-based applications to busy poll when they call epoll_wait. This may
+ not be desirable as many applications may not have the need to busy poll.
+
+2. Applications using recent kernels can issue an ioctl on the epoll context
+ file descriptor to set (``EPIOCSPARAMS``) or get (``EPIOCGPARAMS``) ``struct
+ epoll_params``:, which user programs can define as follows:
+
+.. code-block:: c
+
+ struct epoll_params {
+ uint32_t busy_poll_usecs;
+ uint16_t busy_poll_budget;
+ uint8_t prefer_busy_poll;
+
+ /* pad the struct to a multiple of 64bits */
+ uint8_t __pad;
+ };
+
IRQ mitigation
---------------
@@ -222,12 +289,111 @@ Such applications can pledge to the kernel that they will perform a busy
polling operation periodically, and the driver should keep the device IRQs
permanently masked. This mode is enabled by using the ``SO_PREFER_BUSY_POLL``
socket option. To avoid system misbehavior the pledge is revoked
-if ``gro_flush_timeout`` passes without any busy poll call.
+if ``gro_flush_timeout`` passes without any busy poll call. For epoll-based
+busy polling applications, the ``prefer_busy_poll`` field of ``struct
+epoll_params`` can be set to 1 and the ``EPIOCSPARAMS`` ioctl can be issued to
+enable this mode. See the above section for more details.
The NAPI budget for busy polling is lower than the default (which makes
sense given the low latency intention of normal busy polling). This is
not the case with IRQ mitigation, however, so the budget can be adjusted
-with the ``SO_BUSY_POLL_BUDGET`` socket option.
+with the ``SO_BUSY_POLL_BUDGET`` socket option. For epoll-based busy polling
+applications, the ``busy_poll_budget`` field can be adjusted to the desired value
+in ``struct epoll_params`` and set on a specific epoll context using the ``EPIOCSPARAMS``
+ioctl. See the above section for more details.
+
+It is important to note that choosing a large value for ``gro_flush_timeout``
+will defer IRQs to allow for better batch processing, but will induce latency
+when the system is not fully loaded. Choosing a small value for
+``gro_flush_timeout`` can cause interference of the user application which is
+attempting to busy poll by device IRQs and softirq processing. This value
+should be chosen carefully with these tradeoffs in mind. epoll-based busy
+polling applications may be able to mitigate how much user processing happens
+by choosing an appropriate value for ``maxevents``.
+
+Users may want to consider an alternate approach, IRQ suspension, to help deal
+with these tradeoffs.
+
+IRQ suspension
+--------------
+
+IRQ suspension is a mechanism wherein device IRQs are masked while epoll
+triggers NAPI packet processing.
+
+While application calls to epoll_wait successfully retrieve events, the kernel will
+defer the IRQ suspension timer. If the kernel does not retrieve any events
+while busy polling (for example, because network traffic levels subsided), IRQ
+suspension is disabled and the IRQ mitigation strategies described above are
+engaged.
+
+This allows users to balance CPU consumption with network processing
+efficiency.
+
+To use this mechanism:
+
+ 1. The per-NAPI config parameter ``irq-suspend-timeout`` should be set to the
+ maximum time (in nanoseconds) the application can have its IRQs
+ suspended. This is done using netlink, as described above. This timeout
+ serves as a safety mechanism to restart IRQ driver interrupt processing if
+ the application has stalled. This value should be chosen so that it covers
+ the amount of time the user application needs to process data from its
+ call to epoll_wait, noting that applications can control how much data
+ they retrieve by setting ``max_events`` when calling epoll_wait.
+
+ 2. The sysfs parameter or per-NAPI config parameters ``gro_flush_timeout``
+ and ``napi_defer_hard_irqs`` can be set to low values. They will be used
+ to defer IRQs after busy poll has found no data.
+
+ 3. The ``prefer_busy_poll`` flag must be set to true. This can be done using
+ the ``EPIOCSPARAMS`` ioctl as described above.
+
+ 4. The application uses epoll as described above to trigger NAPI packet
+ processing.
+
+As mentioned above, as long as subsequent calls to epoll_wait return events to
+userland, the ``irq-suspend-timeout`` is deferred and IRQs are disabled. This
+allows the application to process data without interference.
+
+Once a call to epoll_wait results in no events being found, IRQ suspension is
+automatically disabled and the ``gro_flush_timeout`` and
+``napi_defer_hard_irqs`` mitigation mechanisms take over.
+
+It is expected that ``irq-suspend-timeout`` will be set to a value much larger
+than ``gro_flush_timeout`` as ``irq-suspend-timeout`` should suspend IRQs for
+the duration of one userland processing cycle.
+
+While it is not stricly necessary to use ``napi_defer_hard_irqs`` and
+``gro_flush_timeout`` to use IRQ suspension, their use is strongly
+recommended.
+
+IRQ suspension causes the system to alternate between polling mode and
+irq-driven packet delivery. During busy periods, ``irq-suspend-timeout``
+overrides ``gro_flush_timeout`` and keeps the system busy polling, but when
+epoll finds no events, the setting of ``gro_flush_timeout`` and
+``napi_defer_hard_irqs`` determine the next step.
+
+There are essentially three possible loops for network processing and
+packet delivery:
+
+1) hardirq -> softirq -> napi poll; basic interrupt delivery
+2) timer -> softirq -> napi poll; deferred irq processing
+3) epoll -> busy-poll -> napi poll; busy looping
+
+Loop 2 can take control from Loop 1, if ``gro_flush_timeout`` and
+``napi_defer_hard_irqs`` are set.
+
+If ``gro_flush_timeout`` and ``napi_defer_hard_irqs`` are set, Loops 2
+and 3 "wrestle" with each other for control.
+
+During busy periods, ``irq-suspend-timeout`` is used as timer in Loop 2,
+which essentially tilts network processing in favour of Loop 3.
+
+If ``gro_flush_timeout`` and ``napi_defer_hard_irqs`` are not set, Loop 3
+cannot take control from Loop 1.
+
+Therefore, setting ``gro_flush_timeout`` and ``napi_defer_hard_irqs`` is
+the recommended usage, because otherwise setting ``irq-suspend-timeout``
+might not have any discernible effect.
.. _threaded:
--
2.25.1
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [PATCH net-next v6 7/7] docs: networking: Describe irq suspension
2024-11-04 21:55 ` [PATCH net-next v6 7/7] docs: networking: Describe irq suspension Joe Damato
@ 2024-11-05 1:12 ` Bagas Sanjaya
0 siblings, 0 replies; 17+ messages in thread
From: Bagas Sanjaya @ 2024-11-05 1:12 UTC (permalink / raw)
To: Joe Damato, netdev
Cc: corbet, hdanton, pabeni, namangulati, edumazet, amritha.nambiar,
sridhar.samudrala, sdf, peter, m2shafiei, bjorn, hch, willy,
willemdebruijn.kernel, skhawaja, kuba, Martin Karsten,
David S. Miller, Simon Horman, Linux Documentation,
Linux Kernel Mailing List, Linux BPF
[-- Attachment #1: Type: text/plain, Size: 318 bytes --]
On Mon, Nov 04, 2024 at 09:55:31PM +0000, Joe Damato wrote:
> Describe irq suspension, the epoll ioctls, and the tradeoffs of using
> different gro_flush_timeout values.
>
The docs LGTM, thanks!
Reviewed-by: Bagas Sanjaya <bagasdotme@gmail.com>
--
An old man doll... just what I always wanted! - Clara
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply [flat|nested] 17+ messages in thread