[PATCH net-next] dpll: avoid multiple function calls to dump netdev info

[PATCH net-next] dpll: avoid multiple function calls to dump netdev info

[Jakub Kicinski]

Due to compiler oddness and because struct dpll_pin is defined
in a private header we have to call into dpll code to get
the handle for the pin associated with a netdev.
Combine getting the pin pointer and getting the info into
a single function call by having the helpers take a netdev
pointer, rather than expecting a pin.

The exports are note needed, networking core can't be a module.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
BTW is the empty nest if the netdev has no pin intentional?
Should we add a comment explaining why it's there?

CC: vadim.fedorenko@linux.dev
CC: arkadiusz.kubalewski@intel.com
CC: jiri@resnulli.us
---
 drivers/dpll/dpll_core.c    |  5 -----
 drivers/dpll/dpll_netlink.c | 38 +++++++++++++++++++++++--------------
 include/linux/dpll.h        | 19 ++++++-------------
 net/core/rtnetlink.c        |  4 ++--
 4 files changed, 32 insertions(+), 34 deletions(-)

diff --git a/drivers/dpll/dpll_core.c b/drivers/dpll/dpll_core.c
index 12fcd420396e..a87bf95216c1 100644
--- a/drivers/dpll/dpll_core.c
+++ b/drivers/dpll/dpll_core.c
@@ -44,11 +44,6 @@ struct dpll_pin_registration {
 	void *priv;
 };
 
-struct dpll_pin *netdev_dpll_pin(const struct net_device *dev)
-{
-	return rcu_dereference_rtnl(dev->dpll_pin);
-}
-
 struct dpll_device *dpll_device_get_by_id(int id)
 {
 	if (xa_get_mark(&dpll_device_xa, id, DPLL_REGISTERED))
diff --git a/drivers/dpll/dpll_netlink.c b/drivers/dpll/dpll_netlink.c
index 1419fd0d241c..9be4ae35fea2 100644
--- a/drivers/dpll/dpll_netlink.c
+++ b/drivers/dpll/dpll_netlink.c
@@ -8,6 +8,7 @@
  */
 #include <linux/module.h>
 #include <linux/kernel.h>
+#include <linux/netdevice.h>
 #include <net/genetlink.h>
 #include "dpll_core.h"
 #include "dpll_netlink.h"
@@ -47,18 +48,6 @@ dpll_msg_add_dev_parent_handle(struct sk_buff *msg, u32 id)
 	return 0;
 }
 
-/**
- * dpll_msg_pin_handle_size - get size of pin handle attribute for given pin
- * @pin: pin pointer
- *
- * Return: byte size of pin handle attribute for given pin.
- */
-size_t dpll_msg_pin_handle_size(struct dpll_pin *pin)
-{
-	return pin ? nla_total_size(4) : 0; /* DPLL_A_PIN_ID */
-}
-EXPORT_SYMBOL_GPL(dpll_msg_pin_handle_size);
-
 /**
  * dpll_msg_add_pin_handle - attach pin handle attribute to a given message
  * @msg: pointer to sk_buff message to attach a pin handle
@@ -68,7 +57,7 @@ EXPORT_SYMBOL_GPL(dpll_msg_pin_handle_size);
  * * 0 - success
  * * -EMSGSIZE - no space in message to attach pin handle
  */
-int dpll_msg_add_pin_handle(struct sk_buff *msg, struct dpll_pin *pin)
+static int dpll_msg_add_pin_handle(struct sk_buff *msg, struct dpll_pin *pin)
 {
 	if (!pin)
 		return 0;
@@ -76,7 +65,28 @@ int dpll_msg_add_pin_handle(struct sk_buff *msg, struct dpll_pin *pin)
 		return -EMSGSIZE;
 	return 0;
 }
-EXPORT_SYMBOL_GPL(dpll_msg_add_pin_handle);
+
+static struct dpll_pin *netdev_dpll_pin(const struct net_device *dev)
+{
+	return rcu_dereference_rtnl(dev->dpll_pin);
+}
+
+/**
+ * dpll_netdev_pin_handle_size - get size of pin handle attribute of a netdev
+ * @dev: netdev from which to get the pin
+ *
+ * Return: byte size of pin handle attribute, or 0 if @dev has no pin.
+ */
+size_t dpll_netdev_pin_handle_size(const struct net_device *dev)
+{
+	return netdev_dpll_pin(dev) ? nla_total_size(4) : 0; /* DPLL_A_PIN_ID */
+}
+
+int dpll_netdev_add_pin_handle(struct sk_buff *msg,
+			       const struct net_device *dev)
+{
+	return dpll_msg_add_pin_handle(msg, netdev_dpll_pin(dev));
+}
 
 static int
 dpll_msg_add_mode(struct sk_buff *msg, struct dpll_device *dpll,
diff --git a/include/linux/dpll.h b/include/linux/dpll.h
index e3abde993244..ff14d1e88f87 100644
--- a/include/linux/dpll.h
+++ b/include/linux/dpll.h
@@ -123,15 +123,17 @@ struct dpll_pin_properties {
 };
 
 #if IS_ENABLED(CONFIG_DPLL)
-size_t dpll_msg_pin_handle_size(struct dpll_pin *pin);
-int dpll_msg_add_pin_handle(struct sk_buff *msg, struct dpll_pin *pin);
+size_t dpll_netdev_pin_handle_size(const struct net_device *dev);
+int dpll_netdev_add_pin_handle(struct sk_buff *msg,
+			       const struct net_device *dev);
 #else
-static inline size_t dpll_msg_pin_handle_size(struct dpll_pin *pin)
+static inline size_t dpll_netdev_pin_handle_size(const struct net_device *dev)
 {
 	return 0;
 }
 
-static inline int dpll_msg_add_pin_handle(struct sk_buff *msg, struct dpll_pin *pin)
+static inline int
+dpll_netdev_add_pin_handle(struct sk_buff *msg, const struct net_device *dev)
 {
 	return 0;
 }
@@ -170,13 +172,4 @@ int dpll_device_change_ntf(struct dpll_device *dpll);
 
 int dpll_pin_change_ntf(struct dpll_pin *pin);
 
-#if !IS_ENABLED(CONFIG_DPLL)
-static inline struct dpll_pin *netdev_dpll_pin(const struct net_device *dev)
-{
-	return NULL;
-}
-#else
-struct dpll_pin *netdev_dpll_pin(const struct net_device *dev);
-#endif
-
 #endif
diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index 780b330f8ef9..e0353487c57e 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -1056,7 +1056,7 @@ static size_t rtnl_dpll_pin_size(const struct net_device *dev)
 {
 	size_t size = nla_total_size(0); /* nest IFLA_DPLL_PIN */
 
-	size += dpll_msg_pin_handle_size(netdev_dpll_pin(dev));
+	size += dpll_netdev_pin_handle_size(dev);
 
 	return size;
 }
@@ -1793,7 +1793,7 @@ static int rtnl_fill_dpll_pin(struct sk_buff *skb,
 	if (!dpll_pin_nest)
 		return -EMSGSIZE;
 
-	ret = dpll_msg_add_pin_handle(skb, netdev_dpll_pin(dev));
+	ret = dpll_netdev_add_pin_handle(skb, dev);
 	if (ret < 0)
 		goto nest_cancel;
 
-- 
2.43.2

Re: [PATCH net-next] dpll: avoid multiple function calls to dump netdev info

[Jiri Pirko]

Fri, Mar 01, 2024 at 01:16:07AM CET, kuba@kernel.org wrote:
>Due to compiler oddness and because struct dpll_pin is defined
>in a private header we have to call into dpll code to get
>the handle for the pin associated with a netdev.
>Combine getting the pin pointer and getting the info into
>a single function call by having the helpers take a netdev
>pointer, rather than expecting a pin.
>
>The exports are note needed, networking core can't be a module.
>
>Signed-off-by: Jakub Kicinski <kuba@kernel.org>

Reviewed-by: Jiri Pirko <jiri@nvidia.com>


>---
>BTW is the empty nest if the netdev has no pin intentional?

The user can tell if this is or is not supported by kernel easily.

>Should we add a comment explaining why it's there?

Yeah.

Re: [PATCH net-next] dpll: avoid multiple function calls to dump netdev info

[Eric Dumazet]

On Fri, Mar 1, 2024 at 4:40 PM Jiri Pirko <jiri@resnulli.us> wrote:
>
> Fri, Mar 01, 2024 at 01:16:07AM CET, kuba@kernel.org wrote:
> >Due to compiler oddness and because struct dpll_pin is defined
> >in a private header we have to call into dpll code to get
> >the handle for the pin associated with a netdev.
> >Combine getting the pin pointer and getting the info into
> >a single function call by having the helpers take a netdev
> >pointer, rather than expecting a pin.
> >
> >The exports are note needed, networking core can't be a module.
> >
> >Signed-off-by: Jakub Kicinski <kuba@kernel.org>
>
> Reviewed-by: Jiri Pirko <jiri@nvidia.com>
>
>
> >---
> >BTW is the empty nest if the netdev has no pin intentional?
>
> The user can tell if this is or is not supported by kernel easily.

This is a high cost for hosts with hundreds of devices :/

Was it the reason you did not want me to remove zero IFLA_MAP ?

Re: [PATCH net-next] dpll: avoid multiple function calls to dump netdev info

[Jakub Kicinski]

On Fri, 1 Mar 2024 17:24:07 +0100 Eric Dumazet wrote:
> > >BTW is the empty nest if the netdev has no pin intentional?  
> >
> > The user can tell if this is or is not supported by kernel easily.  

Seems legit, although user can also do:

$ genl ctrl list | grep dpll
Name: dpll

> This is a high cost for hosts with hundreds of devices :/

right, a bit high cost for a relatively rare feature :(

Re: [PATCH net-next] dpll: avoid multiple function calls to dump netdev info

[Jiri Pirko]

Fri, Mar 01, 2024 at 06:31:28PM CET, kuba@kernel.org wrote:
>On Fri, 1 Mar 2024 17:24:07 +0100 Eric Dumazet wrote:
>> > >BTW is the empty nest if the netdev has no pin intentional?  
>> >
>> > The user can tell if this is or is not supported by kernel easily.  
>
>Seems legit, although user can also do:
>
>$ genl ctrl list | grep dpll
>Name: dpll

True.

>
>> This is a high cost for hosts with hundreds of devices :/
>
>right, a bit high cost for a relatively rare feature :(

I agree. Let's remove the empty nest then.