| Message ID | 20231227140057.174314-2-stanislaw.gruszka@linux.intel.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | [1/2] thermal: netlink: Add enum for mutlicast groups indexes | expand |
Hi Stanislaw, thanks for this optimization. One question below. On Wed, Dec 27, 2023 at 03:00:57PM +0100, Stanislaw Gruszka wrote: > Add a helper function to check if there are listeners for > thermal_gnl_family multicast groups. > > For now use it to avoid unnecessary allocations and sending > thermal genl messages when there are no recipients. > > In the future, in conjunction with (not yet implemented) notification > of change in the netlink socket group membership, this helper can be > used to open/close hardware interfaces based on the presence of > user space subscribers. > > Signed-off-by: Stanislaw Gruszka <stanislaw.gruszka@linux.intel.com> > --- > drivers/thermal/thermal_netlink.c | 11 +++++++++++ > 1 file changed, 11 insertions(+) > > diff --git a/drivers/thermal/thermal_netlink.c b/drivers/thermal/thermal_netlink.c > index aca36c4ddbf3..b4e758d22077 100644 > --- a/drivers/thermal/thermal_netlink.c > +++ b/drivers/thermal/thermal_netlink.c > @@ -76,6 +76,11 @@ typedef int (*cb_t)(struct param *); > > static struct genl_family thermal_gnl_family; > > +static int thermal_group_has_listeners(enum thermal_genl_multicast_groups group) > +{ > + return genl_has_listeners(&thermal_gnl_family, &init_net, group); > +} > + > /************************** Sampling encoding *******************************/ > > int thermal_genl_sampling_temp(int id, int temp) > @@ -83,6 +88,9 @@ int thermal_genl_sampling_temp(int id, int temp) > struct sk_buff *skb; > void *hdr; > > + if (!thermal_group_has_listeners(THERMAL_GENL_SAMPLING_GROUP)) > + return -ESRCH; > + Do really want to return an error ? Shall we just bail out instead ? [ ... ]
Hi Daniel, On Wed, Dec 27, 2023 at 03:11:03PM +0100, Daniel Lezcano wrote: > > +static int thermal_group_has_listeners(enum thermal_genl_multicast_groups group) > > +{ > > + return genl_has_listeners(&thermal_gnl_family, &init_net, group); > > +} > > + > > /************************** Sampling encoding *******************************/ > > > > int thermal_genl_sampling_temp(int id, int temp) > > @@ -83,6 +88,9 @@ int thermal_genl_sampling_temp(int id, int temp) > > struct sk_buff *skb; > > void *hdr; > > > > + if (!thermal_group_has_listeners(THERMAL_GENL_SAMPLING_GROUP)) > > + return -ESRCH; > > + > > Do really want to return an error ? Shall we just bail out instead ? I decided for error because thermal_notify_* are int functions and we return error code for some other cases when the messages can not be sent (i.e. alloc error). Event if currently return value is ignored by all callers (FWICT), error information could be used theoretically. If returning 0 is preferable/better, I can change that. Regards Stanislaw
On 27/12/2023 16:31, Stanislaw Gruszka wrote: > Hi Daniel, > > On Wed, Dec 27, 2023 at 03:11:03PM +0100, Daniel Lezcano wrote: >>> +static int thermal_group_has_listeners(enum thermal_genl_multicast_groups group) >>> +{ >>> + return genl_has_listeners(&thermal_gnl_family, &init_net, group); >>> +} >>> + >>> /************************** Sampling encoding *******************************/ >>> >>> int thermal_genl_sampling_temp(int id, int temp) >>> @@ -83,6 +88,9 @@ int thermal_genl_sampling_temp(int id, int temp) >>> struct sk_buff *skb; >>> void *hdr; >>> >>> + if (!thermal_group_has_listeners(THERMAL_GENL_SAMPLING_GROUP)) >>> + return -ESRCH; >>> + >> >> Do really want to return an error ? Shall we just bail out instead ? > > I decided for error because thermal_notify_* are int functions and we > return error code for some other cases when the messages can not be > sent (i.e. alloc error). > Event if currently return value is ignored by all callers (FWICT), > error information could be used theoretically. > > If returning 0 is preferable/better, I can change that. The caller will have to handle the specific case if there is no listeners. There is an error if the message can not be sent but here there is no error related to that, just we don't send it. So having an error when there is nothing to do is not really an error.
On Wed, Dec 27, 2023 at 04:40:31PM +0100, Daniel Lezcano wrote: > On 27/12/2023 16:31, Stanislaw Gruszka wrote: > > Hi Daniel, > > > > On Wed, Dec 27, 2023 at 03:11:03PM +0100, Daniel Lezcano wrote: > > > > +static int thermal_group_has_listeners(enum thermal_genl_multicast_groups group) > > > > +{ > > > > + return genl_has_listeners(&thermal_gnl_family, &init_net, group); > > > > +} > > > > + > > > > /************************** Sampling encoding *******************************/ > > > > int thermal_genl_sampling_temp(int id, int temp) > > > > @@ -83,6 +88,9 @@ int thermal_genl_sampling_temp(int id, int temp) > > > > struct sk_buff *skb; > > > > void *hdr; > > > > + if (!thermal_group_has_listeners(THERMAL_GENL_SAMPLING_GROUP)) > > > > + return -ESRCH; > > > > + > > > > > > Do really want to return an error ? Shall we just bail out instead ? > > > > I decided for error because thermal_notify_* are int functions and we > > return error code for some other cases when the messages can not be > > sent (i.e. alloc error). > > Event if currently return value is ignored by all callers (FWICT), > > error information could be used theoretically. > > > > If returning 0 is preferable/better, I can change that. > > The caller will have to handle the specific case if there is no listeners. > There is an error if the message can not be sent but here there is no error > related to that, just we don't send it. So having an error when there is > nothing to do is not really an error. Ok, will change to 0 in v2. Regards Stanislaw
diff --git a/drivers/thermal/thermal_netlink.c b/drivers/thermal/thermal_netlink.c index aca36c4ddbf3..b4e758d22077 100644 --- a/drivers/thermal/thermal_netlink.c +++ b/drivers/thermal/thermal_netlink.c @@ -76,6 +76,11 @@ typedef int (*cb_t)(struct param *); static struct genl_family thermal_gnl_family; +static int thermal_group_has_listeners(enum thermal_genl_multicast_groups group) +{ + return genl_has_listeners(&thermal_gnl_family, &init_net, group); +} + /************************** Sampling encoding *******************************/ int thermal_genl_sampling_temp(int id, int temp) @@ -83,6 +88,9 @@ int thermal_genl_sampling_temp(int id, int temp) struct sk_buff *skb; void *hdr; + if (!thermal_group_has_listeners(THERMAL_GENL_SAMPLING_GROUP)) + return -ESRCH; + skb = genlmsg_new(NLMSG_GOODSIZE, GFP_KERNEL); if (!skb) return -ENOMEM; @@ -280,6 +288,9 @@ static int thermal_genl_send_event(enum thermal_genl_event event, int ret = -EMSGSIZE; void *hdr; + if (!thermal_group_has_listeners(THERMAL_GENL_EVENT_GROUP)) + return -ESRCH; + msg = genlmsg_new(NLMSG_GOODSIZE, GFP_KERNEL); if (!msg) return -ENOMEM;
Add a helper function to check if there are listeners for thermal_gnl_family multicast groups. For now use it to avoid unnecessary allocations and sending thermal genl messages when there are no recipients. In the future, in conjunction with (not yet implemented) notification of change in the netlink socket group membership, this helper can be used to open/close hardware interfaces based on the presence of user space subscribers. Signed-off-by: Stanislaw Gruszka <stanislaw.gruszka@linux.intel.com> --- drivers/thermal/thermal_netlink.c | 11 +++++++++++ 1 file changed, 11 insertions(+)