Support dumping defer_hard_irqs for a NAPI ID.
Signed-off-by: Joe Damato <jdamato@fastly.com>
Reviewed-by: Martin Karsten <mkarsten@uwaterloo.ca>
Tested-by: Martin Karsten <mkarsten@uwaterloo.ca>
---
Documentation/netlink/specs/netdev.yaml | 6 ++++++
include/uapi/linux/netdev.h | 1 +
net/core/netdev-genl.c | 5 +++++
tools/include/uapi/linux/netdev.h | 1 +
4 files changed, 13 insertions(+)
diff --git a/Documentation/netlink/specs/netdev.yaml b/Documentation/netlink/specs/netdev.yaml
index 959755be4d7f..ee4f99fd4574 100644
--- a/Documentation/netlink/specs/netdev.yaml
+++ b/Documentation/netlink/specs/netdev.yaml
@@ -244,6 +244,11 @@ attribute-sets:
threaded mode. If NAPI is not in threaded mode (i.e. uses normal
softirq context), the attribute will be absent.
type: u32
+ -
+ name: defer-hard-irqs
+ doc: The number of consecutive empty polls before IRQ deferral ends
+ and hardware IRQs are re-enabled.
+ type: s32
-
name: queue
attributes:
@@ -593,6 +598,7 @@ operations:
- ifindex
- irq
- pid
+ - defer-hard-irqs
dump:
request:
attributes:
diff --git a/include/uapi/linux/netdev.h b/include/uapi/linux/netdev.h
index 43742ac5b00d..43bb1aad9611 100644
--- a/include/uapi/linux/netdev.h
+++ b/include/uapi/linux/netdev.h
@@ -121,6 +121,7 @@ enum {
NETDEV_A_NAPI_ID,
NETDEV_A_NAPI_IRQ,
NETDEV_A_NAPI_PID,
+ NETDEV_A_NAPI_DEFER_HARD_IRQS,
__NETDEV_A_NAPI_MAX,
NETDEV_A_NAPI_MAX = (__NETDEV_A_NAPI_MAX - 1)
diff --git a/net/core/netdev-genl.c b/net/core/netdev-genl.c
index 05f9515d2c05..79b14ad6f4bb 100644
--- a/net/core/netdev-genl.c
+++ b/net/core/netdev-genl.c
@@ -160,6 +160,7 @@ static int
netdev_nl_napi_fill_one(struct sk_buff *rsp, struct napi_struct *napi,
const struct genl_info *info)
{
+ int napi_defer_hard_irqs;
void *hdr;
pid_t pid;
@@ -188,6 +189,10 @@ netdev_nl_napi_fill_one(struct sk_buff *rsp, struct napi_struct *napi,
goto nla_put_failure;
}
+ napi_defer_hard_irqs = napi_get_defer_hard_irqs(napi);
+ if (nla_put_s32(rsp, NETDEV_A_NAPI_DEFER_HARD_IRQS, napi_defer_hard_irqs))
+ goto nla_put_failure;
+
genlmsg_end(rsp, hdr);
return 0;
diff --git a/tools/include/uapi/linux/netdev.h b/tools/include/uapi/linux/netdev.h
index 43742ac5b00d..43bb1aad9611 100644
--- a/tools/include/uapi/linux/netdev.h
+++ b/tools/include/uapi/linux/netdev.h
@@ -121,6 +121,7 @@ enum {
NETDEV_A_NAPI_ID,
NETDEV_A_NAPI_IRQ,
NETDEV_A_NAPI_PID,
+ NETDEV_A_NAPI_DEFER_HARD_IRQS,
__NETDEV_A_NAPI_MAX,
NETDEV_A_NAPI_MAX = (__NETDEV_A_NAPI_MAX - 1)
--
2.25.1On Thu, 29 Aug 2024 13:11:58 +0000 Joe Damato wrote: > diff --git a/Documentation/netlink/specs/netdev.yaml b/Documentation/netlink/specs/netdev.yaml > index 959755be4d7f..ee4f99fd4574 100644 > --- a/Documentation/netlink/specs/netdev.yaml > +++ b/Documentation/netlink/specs/netdev.yaml > @@ -244,6 +244,11 @@ attribute-sets: > threaded mode. If NAPI is not in threaded mode (i.e. uses normal > softirq context), the attribute will be absent. > type: u32 > + - > + name: defer-hard-irqs > + doc: The number of consecutive empty polls before IRQ deferral ends > + and hardware IRQs are re-enabled. > + type: s32 Why is this a signed value? 🤔️ You can use: check: max: s32-max to have netlink validate the overflow if you switch to u32. > - > name: queue > attributes: > @@ -188,6 +189,10 @@ netdev_nl_napi_fill_one(struct sk_buff *rsp, struct napi_struct *napi, > goto nla_put_failure; > } > > + napi_defer_hard_irqs = napi_get_defer_hard_irqs(napi); Here, for example the READ_ONCE() wouldn't have been necessary, right? Cause we are holding rtnl_lock(), just a random thought, not really actionable. > + if (nla_put_s32(rsp, NETDEV_A_NAPI_DEFER_HARD_IRQS, napi_defer_hard_irqs)) > + goto nla_put_failure;
On Thu, Aug 29, 2024 at 03:08:28PM -0700, Jakub Kicinski wrote:
> On Thu, 29 Aug 2024 13:11:58 +0000 Joe Damato wrote:
> > diff --git a/Documentation/netlink/specs/netdev.yaml b/Documentation/netlink/specs/netdev.yaml
> > index 959755be4d7f..ee4f99fd4574 100644
> > --- a/Documentation/netlink/specs/netdev.yaml
> > +++ b/Documentation/netlink/specs/netdev.yaml
> > @@ -244,6 +244,11 @@ attribute-sets:
> > threaded mode. If NAPI is not in threaded mode (i.e. uses normal
> > softirq context), the attribute will be absent.
> > type: u32
> > + -
> > + name: defer-hard-irqs
> > + doc: The number of consecutive empty polls before IRQ deferral ends
> > + and hardware IRQs are re-enabled.
> > + type: s32
>
> Why is this a signed value? 🤔️
In commit 6f8b12d661d0 ("net: napi: add hard irqs deferral
feature"), napi_defer_hard_irqs was added to struct net_device as an
int. I was trying to match that and thus made the field a signed int
in the napi struct, as well.
It looks like there was a possibility of overflow introduced in that
commit in change_napi_defer_hard_irqs maybe ?
If you'd prefer I could:
- submit a Fixes to change the net_device field to a u32 and then
change the netlink code to also be u32
- add an overflow check (val > U32_MAX) in
change_napi_defer_hard_irqs
Which would mean for the v2 of this series:
- drop the overflow check I added in Patch 1
- Change netlink to use u32 in this patch
What do you think?
> You can use:
>
> check:
> max: s32-max
>
> to have netlink validate the overflow if you switch to u32.
>
> > -
> > name: queue
> > attributes:
>
> > @@ -188,6 +189,10 @@ netdev_nl_napi_fill_one(struct sk_buff *rsp, struct napi_struct *napi,
> > goto nla_put_failure;
> > }
> >
> > + napi_defer_hard_irqs = napi_get_defer_hard_irqs(napi);
>
> Here, for example the READ_ONCE() wouldn't have been necessary, right?
> Cause we are holding rtnl_lock(), just a random thought, not really
> actionable.
That's right, yes.
I think it depends on where we land with the wrapper functions? I'll
reply with my thoughts about that in that thread.
On Fri, 30 Aug 2024 10:10:47 +0100 Joe Damato wrote:
> > > + name: defer-hard-irqs
> > > + doc: The number of consecutive empty polls before IRQ deferral ends
> > > + and hardware IRQs are re-enabled.
> > > + type: s32
> >
> > Why is this a signed value? 🤔️
>
> In commit 6f8b12d661d0 ("net: napi: add hard irqs deferral
> feature"), napi_defer_hard_irqs was added to struct net_device as an
> int. I was trying to match that and thus made the field a signed int
> in the napi struct, as well.
It's probably because int is the default type in C.
The choice of types in netlink feels more deliberate.
> It looks like there was a possibility of overflow introduced in that
> commit in change_napi_defer_hard_irqs maybe ?
>
> If you'd prefer I could:
> - submit a Fixes to change the net_device field to a u32 and then
> change the netlink code to also be u32
> - add an overflow check (val > U32_MAX) in
> change_napi_defer_hard_irqs
>
> Which would mean for the v2 of this series:
> - drop the overflow check I added in Patch 1
> - Change netlink to use u32 in this patch
>
> What do you think?
Whether we want to clean things up internally is up to you, the overflow
check you're adding in sysfs seems good. We can use u32 in netlink, with
a check: max: s32-max and lift this requirement later if we ever need
the 32nd bit?
On Fri, Aug 30, 2024 at 01:28:08PM -0700, Jakub Kicinski wrote:
> On Fri, 30 Aug 2024 10:10:47 +0100 Joe Damato wrote:
> > > > + name: defer-hard-irqs
> > > > + doc: The number of consecutive empty polls before IRQ deferral ends
> > > > + and hardware IRQs are re-enabled.
> > > > + type: s32
> > >
> > > Why is this a signed value? 🤔️
> >
> > In commit 6f8b12d661d0 ("net: napi: add hard irqs deferral
> > feature"), napi_defer_hard_irqs was added to struct net_device as an
> > int. I was trying to match that and thus made the field a signed int
> > in the napi struct, as well.
>
> It's probably because int is the default type in C.
> The choice of types in netlink feels more deliberate.
>
> > It looks like there was a possibility of overflow introduced in that
> > commit in change_napi_defer_hard_irqs maybe ?
> >
> > If you'd prefer I could:
> > - submit a Fixes to change the net_device field to a u32 and then
> > change the netlink code to also be u32
> > - add an overflow check (val > U32_MAX) in
> > change_napi_defer_hard_irqs
> >
> > Which would mean for the v2 of this series:
> > - drop the overflow check I added in Patch 1
> > - Change netlink to use u32 in this patch
> >
> > What do you think?
>
> Whether we want to clean things up internally is up to you, the overflow
> check you're adding in sysfs seems good. We can use u32 in netlink, with
> a check: max: s32-max and lift this requirement later if we ever need
> the 32nd bit?
OK, u32 + check for max: s32-max seems good.
Is the overflow check in sysfs a fixes I send separately or can I
sneak that into this series?
On Fri, 30 Aug 2024 21:31:25 +0100 Joe Damato wrote: > Is the overflow check in sysfs a fixes I send separately or can I > sneak that into this series? It does look kinda random in that patch, I'd send separately
© 2016 - 2026 Red Hat, Inc.