Re: [ovs-dev] [PATCH net-next 03/10] genetlink: remove userhdr from struct genl_info

2023-08-10 Thread Jiri Pirko
Wed, Aug 09, 2023 at 08:26:41PM CEST, k...@kernel.org wrote:
>Only three families use info->userhdr and fixed headers
>are discouraged for new families. So remove the pointer
>from struct genl_info to save some space. Compute
>the header pointer at runtime. Saved space will be used
>for a family pointer in later patches.
>
>Signed-off-by: Jakub Kicinski 

Reviewed-by: Jiri Pirko 

I'm fine with the existing message, but what Johannes suggests is also
ok.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev


Re: [ovs-dev] [PATCH net-next 03/10] genetlink: remove userhdr from struct genl_info

2023-08-09 Thread Jakub Kicinski
On Wed, 09 Aug 2023 22:59:47 +0200 Johannes Berg wrote:
> On Wed, 2023-08-09 at 11:26 -0700, Jakub Kicinski wrote:
> > Only three families use info->userhdr and fixed headers
> > are discouraged for new families. So remove the pointer
> > from struct genl_info to save some space. Compute
> > the header pointer at runtime. Saved space will be used
> > for a family pointer in later patches.  
> 
> Seems fine to me, but I'm not sure I buy the rationale that it's for
> saving space - it's a single pointer on the stack? I'd probably argue
> the computation being pointless for basically everyone except for a
> handful users?

Fair, I'll update all the commit messages.

> Reviewed-by: Johannes Berg 

Thanks!
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev


Re: [ovs-dev] [PATCH net-next 03/10] genetlink: remove userhdr from struct genl_info

2023-08-09 Thread Johannes Berg
On Wed, 2023-08-09 at 11:26 -0700, Jakub Kicinski wrote:
> Only three families use info->userhdr and fixed headers
> are discouraged for new families. So remove the pointer
> from struct genl_info to save some space. Compute
> the header pointer at runtime. Saved space will be used
> for a family pointer in later patches.

Seems fine to me, but I'm not sure I buy the rationale that it's for
saving space - it's a single pointer on the stack? I'd probably argue
the computation being pointless for basically everyone except for a
handful users?

Reviewed-by: Johannes Berg 

johannes
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev


[ovs-dev] [PATCH net-next 03/10] genetlink: remove userhdr from struct genl_info

2023-08-09 Thread Jakub Kicinski
Only three families use info->userhdr and fixed headers
are discouraged for new families. So remove the pointer
from struct genl_info to save some space. Compute
the header pointer at runtime. Saved space will be used
for a family pointer in later patches.

Signed-off-by: Jakub Kicinski 
---
CC: philipp.reis...@linbit.com
CC: lars.ellenb...@linbit.com
CC: christoph.boehmwal...@linbit.com
CC: ax...@kernel.dk
CC: pshe...@ovn.org
CC: jma...@redhat.com
CC: ying@windriver.com
CC: jacob.e.kel...@intel.com
CC: drbd-...@lists.linbit.com
CC: linux-bl...@vger.kernel.org
CC: d...@openvswitch.org
CC: tipc-discuss...@lists.sourceforge.net
---
 drivers/block/drbd/drbd_nl.c |  9 +
 include/net/genetlink.h  |  7 +--
 net/netlink/genetlink.c  |  1 -
 net/openvswitch/conntrack.c  |  2 +-
 net/openvswitch/datapath.c   | 29 -
 net/openvswitch/meter.c  | 10 +-
 net/tipc/netlink_compat.c|  2 +-
 7 files changed, 33 insertions(+), 27 deletions(-)

diff --git a/drivers/block/drbd/drbd_nl.c b/drivers/block/drbd/drbd_nl.c
index cddae6f4b00f..d3538bd83fb3 100644
--- a/drivers/block/drbd/drbd_nl.c
+++ b/drivers/block/drbd/drbd_nl.c
@@ -159,7 +159,7 @@ static int drbd_msg_sprintf_info(struct sk_buff *skb, const 
char *fmt, ...)
 static int drbd_adm_prepare(struct drbd_config_context *adm_ctx,
struct sk_buff *skb, struct genl_info *info, unsigned flags)
 {
-   struct drbd_genlmsghdr *d_in = info->userhdr;
+   struct drbd_genlmsghdr *d_in = genl_info_userhdr(info);
const u8 cmd = info->genlhdr->cmd;
int err;
 
@@ -1396,8 +1396,9 @@ static void drbd_suspend_al(struct drbd_device *device)
 
 static bool should_set_defaults(struct genl_info *info)
 {
-   unsigned flags = ((struct drbd_genlmsghdr*)info->userhdr)->flags;
-   return 0 != (flags & DRBD_GENL_F_SET_DEFAULTS);
+   struct drbd_genlmsghdr *dh = genl_info_userhdr(info);
+
+   return 0 != (dh->flags & DRBD_GENL_F_SET_DEFAULTS);
 }
 
 static unsigned int drbd_al_extents_max(struct drbd_backing_dev *bdev)
@@ -4276,7 +4277,7 @@ static void device_to_info(struct device_info *info,
 int drbd_adm_new_minor(struct sk_buff *skb, struct genl_info *info)
 {
struct drbd_config_context adm_ctx;
-   struct drbd_genlmsghdr *dh = info->userhdr;
+   struct drbd_genlmsghdr *dh = genl_info_userhdr(info);
enum drbd_ret_code retcode;
 
retcode = drbd_adm_prepare(_ctx, skb, info, DRBD_ADM_NEED_RESOURCE);
diff --git a/include/net/genetlink.h b/include/net/genetlink.h
index 0366d0925596..9dc21ec15734 100644
--- a/include/net/genetlink.h
+++ b/include/net/genetlink.h
@@ -95,7 +95,6 @@ struct genl_family {
  * @snd_portid: netlink portid of sender
  * @nlhdr: netlink message header
  * @genlhdr: generic netlink message header
- * @userhdr: user specific header
  * @attrs: netlink attributes
  * @_net: network namespace
  * @user_ptr: user pointers
@@ -106,7 +105,6 @@ struct genl_info {
u32 snd_portid;
const struct nlmsghdr * nlhdr;
struct genlmsghdr * genlhdr;
-   void *  userhdr;
struct nlattr **attrs;
possible_net_t  _net;
void *  user_ptr[2];
@@ -123,6 +121,11 @@ static inline void genl_info_net_set(struct genl_info 
*info, struct net *net)
write_pnet(>_net, net);
 }
 
+static inline void *genl_info_userhdr(const struct genl_info *info)
+{
+   return (u8 *)info->genlhdr + GENL_HDRLEN;
+}
+
 #define GENL_SET_ERR_MSG(info, msg) NL_SET_ERR_MSG((info)->extack, msg)
 
 #define GENL_SET_ERR_MSG_FMT(info, msg, args...) \
diff --git a/net/netlink/genetlink.c b/net/netlink/genetlink.c
index 0d4285688ab9..f98f730bb245 100644
--- a/net/netlink/genetlink.c
+++ b/net/netlink/genetlink.c
@@ -943,7 +943,6 @@ static int genl_family_rcv_msg_doit(const struct 
genl_family *family,
info.snd_portid = NETLINK_CB(skb).portid;
info.nlhdr = nlh;
info.genlhdr = nlmsg_data(nlh);
-   info.userhdr = nlmsg_data(nlh) + GENL_HDRLEN;
info.attrs = attrbuf;
info.extack = extack;
genl_info_net_set(, net);
diff --git a/net/openvswitch/conntrack.c b/net/openvswitch/conntrack.c
index fa955e892210..03a6220b7d99 100644
--- a/net/openvswitch/conntrack.c
+++ b/net/openvswitch/conntrack.c
@@ -1604,7 +1604,7 @@ static struct sk_buff *
 ovs_ct_limit_cmd_reply_start(struct genl_info *info, u8 cmd,
 struct ovs_header **ovs_reply_header)
 {
-   struct ovs_header *ovs_header = info->userhdr;
+   struct ovs_header *ovs_header = genl_info_userhdr(info);
struct sk_buff *skb;
 
skb = genlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
diff --git a/net/openvswitch/datapath.c b/net/openvswitch/datapath.c
index a6d2a0b1aa21..80f5f755b0e6 100644
--- a/net/openvswitch/datapath.c
+++ b/net/openvswitch/datapath.c
@@ -589,7 +589,7 @@ static int queue_userspace_packet(struct datapath *dp,