| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <0455b0ed46dbac54feb13a27b8fede80980b9426.camel@redhat.com>
Date: Mon, 27 Nov 2023 16:00:42 +0100
From: Paolo Abeni <pabeni@...hat.com>
To: Jiri Pirko <jiri@...nulli.us>
Cc: netdev@...r.kernel.org, kuba@...nel.org, davem@...emloft.net,
edumazet@...gle.com, jacob.e.keller@...el.com, jhs@...atatu.com,
johannes@...solutions.net, andriy.shevchenko@...ux.intel.com,
amritha.nambiar@...el.com, sdf@...gle.com, horms@...nel.org
Subject: Re: [patch net-next v4 3/9] devlink: send notifications only if
there are listeners
On Mon, 2023-11-27 at 13:04 +0100, Jiri Pirko wrote:
> Mon, Nov 27, 2023 at 12:01:10PM CET, pabeni@...hat.com wrote:
> > On Thu, 2023-11-23 at 19:15 +0100, Jiri Pirko wrote:
> > > From: Jiri Pirko <jiri@...dia.com>
> > >
> > > Introduce devlink_nl_notify_need() helper and using it to check at the
> > > beginning of notification functions to avoid overhead of composing
> > > notification messages in case nobody listens.
> > >
> > > Signed-off-by: Jiri Pirko <jiri@...dia.com>
> > > ---
> > > net/devlink/dev.c | 5 ++++-
> > > net/devlink/devl_internal.h | 6 ++++++
> > > net/devlink/health.c | 3 +++
> > > net/devlink/linecard.c | 2 +-
> > > net/devlink/param.c | 2 +-
> > > net/devlink/port.c | 2 +-
> > > net/devlink/rate.c | 2 +-
> > > net/devlink/region.c | 2 +-
> > > net/devlink/trap.c | 6 +++---
> > > 9 files changed, 21 insertions(+), 9 deletions(-)
> > >
> > > diff --git a/net/devlink/dev.c b/net/devlink/dev.c
> > > index 7c7517e26862..46407689ef70 100644
> > > --- a/net/devlink/dev.c
> > > +++ b/net/devlink/dev.c
> > > @@ -204,6 +204,9 @@ static void devlink_notify(struct devlink *devlink, enum devlink_command cmd)
> > > WARN_ON(cmd != DEVLINK_CMD_NEW && cmd != DEVLINK_CMD_DEL);
> > > WARN_ON(!devl_is_registered(devlink));
> >
> > minor nit: possibly use ASSERT_DEVLINK_REGISTERED(devlink) above?
>
> Sure, but unrelated to this patch.
>
>
> >
> > >
> > > + if (!devlink_nl_notify_need(devlink))
> > > + return;
> > > +
> > > msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
> > > if (!msg)
> > > return;
> > > @@ -985,7 +988,7 @@ static void __devlink_flash_update_notify(struct devlink *devlink,
> > > cmd != DEVLINK_CMD_FLASH_UPDATE_END &&
> > > cmd != DEVLINK_CMD_FLASH_UPDATE_STATUS);
> > >
> > > - if (!devl_is_registered(devlink))
> > > + if (!devl_is_registered(devlink) || !devlink_nl_notify_need(devlink))
> > > return;
> > >
> > > msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
> > > diff --git a/net/devlink/devl_internal.h b/net/devlink/devl_internal.h
> > > index 59ae4761d10a..510990de094e 100644
> > > --- a/net/devlink/devl_internal.h
> > > +++ b/net/devlink/devl_internal.h
> > > @@ -185,6 +185,12 @@ int devlink_nl_put_nested_handle(struct sk_buff *msg, struct net *net,
> > > struct devlink *devlink, int attrtype);
> > > int devlink_nl_msg_reply_and_new(struct sk_buff **msg, struct genl_info *info);
> > >
> > > +static inline bool devlink_nl_notify_need(struct devlink *devlink)
> > > +{
> > > + return genl_has_listeners(&devlink_nl_family, devlink_net(devlink),
> > > + DEVLINK_MCGRP_CONFIG);
> > > +}
> > > +
> > > /* Notify */
> > > void devlink_notify_register(struct devlink *devlink);
> > > void devlink_notify_unregister(struct devlink *devlink);
> > > diff --git a/net/devlink/health.c b/net/devlink/health.c
> > > index 71ae121dc739..0795dcf22ca8 100644
> > > --- a/net/devlink/health.c
> > > +++ b/net/devlink/health.c
> > > @@ -496,6 +496,9 @@ static void devlink_recover_notify(struct devlink_health_reporter *reporter,
> > > WARN_ON(cmd != DEVLINK_CMD_HEALTH_REPORTER_RECOVER);
> > > ASSERT_DEVLINK_REGISTERED(devlink);
> > >
> > > + if (!devlink_nl_notify_need(devlink))
> > > + return;
> > > +
> > > msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
> > > if (!msg)
> > > return;
> > > diff --git a/net/devlink/linecard.c b/net/devlink/linecard.c
> > > index 9d080ac1734b..45b36975ee6f 100644
> > > --- a/net/devlink/linecard.c
> > > +++ b/net/devlink/linecard.c
> > > @@ -136,7 +136,7 @@ static void devlink_linecard_notify(struct devlink_linecard *linecard,
> > > WARN_ON(cmd != DEVLINK_CMD_LINECARD_NEW &&
> > > cmd != DEVLINK_CMD_LINECARD_DEL);
> > >
> > > - if (!__devl_is_registered(devlink))
> > > + if (!__devl_is_registered(devlink) || !devlink_nl_notify_need(devlink))
> > > return;
> > >
> > > msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
> > > diff --git a/net/devlink/param.c b/net/devlink/param.c
> > > index d74df09311a9..6bb6aee5d937 100644
> > > --- a/net/devlink/param.c
> > > +++ b/net/devlink/param.c
> > > @@ -343,7 +343,7 @@ static void devlink_param_notify(struct devlink *devlink,
> > > * will replay the notifications if the params are added/removed
> > > * outside of the lifetime of the instance.
> > > */
> > > - if (!devl_is_registered(devlink))
> > > + if (!devlink_nl_notify_need(devlink) || !devl_is_registered(devlink))
> >
> > Minor nit: this is the only statement using this order, perhaps swap
> > the tests for consistency?
>
> Right. If respin is needed, I'll swap.
>
>
> >
> > Also possibly add the devlink_nl_notify_need() check in
> > devl_is_registered to reduce code duplication? plus a
>
> It would be odd to have devlink_nl_notify_need() called from
> devl_is_registered().
Sorry for the confusion, out-of-order on my side. What I really mean
is: add __devl_is_registered() in devlink_nl_notify_need().
> Also, it is non only used on notification paths.
> I thought about putting the checks in one function, but those are 2
> separate and unrelated checks, so better to keep them separate.
It looks like devlink_nl_notify_need() implies/requires
__devl_is_registered() ?
Cheers,
Paolo
Powered by blists - more mailing lists