From: Jakub Kicinski <kuba@kernel•org>
To: Jiri Pirko <jiri@resnulli•us>
Cc: netdev@vger•kernel.org, pabeni@redhat•com, davem@davemloft•net,
edumazet@google•com, jacob.e.keller@intel•com, jhs@mojatatu•com,
johannes@sipsolutions•net, andriy.shevchenko@linux•intel.com,
amritha.nambiar@intel•com, sdf@google•com, horms@kernel•org,
przemyslaw.kitszel@intel•com
Subject: Re: [patch net-next v5 5/9] genetlink: introduce per-sock family private storage
Date: Thu, 7 Dec 2023 18:55:26 -0800 [thread overview]
Message-ID: <20231207185526.5e59ab53@kernel.org> (raw)
In-Reply-To: <20231206182120.957225-6-jiri@resnulli.us>
On Wed, 6 Dec 2023 19:21:16 +0100 Jiri Pirko wrote:
> diff --git a/include/net/genetlink.h b/include/net/genetlink.h
> index e18a4c0d69ee..dbf11464e96a 100644
> --- a/include/net/genetlink.h
> +++ b/include/net/genetlink.h
> @@ -87,6 +87,9 @@ struct genl_family {
> int id;
> /* starting number of multicast group IDs in this family */
> unsigned int mcgrp_offset;
> + size_t sock_priv_size;
> + void (*sock_priv_init)(void *priv);
> + void (*sock_priv_destroy)(void *priv);
👍️
but I think it should be above the private fields (and have kdoc)
The families are expected to make use the new fields, and are not
supposed to touch anything private.
> --- a/net/netlink/af_netlink.h
> +++ b/net/netlink/af_netlink.h
> @@ -60,6 +60,21 @@ static inline struct netlink_sock *nlk_sk(struct sock *sk)
>
> #define nlk_test_bit(nr, sk) test_bit(NETLINK_F_##nr, &nlk_sk(sk)->flags)
>
> +struct genl_sock {
> + struct netlink_sock nlk_sk;
> + struct xarray *family_privs;
> +};
> +
> +static inline struct genl_sock *genl_sk(struct sock *sk)
> +{
> + return container_of(nlk_sk(sk), struct genl_sock, nlk_sk);
> +}
> +
> +/* Size of netlink sock is size of the biggest user with priv,
> + * which is currently just Generic Netlink.
> + */
> +#define NETLINK_SOCK_SIZE sizeof(struct genl_sock)
Would feel a little cleaner to me to add
#define NETLINK_SOCK_PROTO_SIZE 8
add that to the size, build time check that struct genl_sock's
size is <= than sizeof(struct netlink_sock) + NETLINK_SOCK_PROTO_SIZE
This way we don't have to fumble the layering by putting genl stuff
in af_netlink.h
> +struct genl_sk_priv {
> + void (*destructor)(void *priv);
> + long priv[];
> +};
> +
> +static struct genl_sk_priv *genl_sk_priv_alloc(struct genl_family *family)
> +{
> + struct genl_sk_priv *priv;
> +
> + priv = kzalloc(size_add(sizeof(*priv), family->sock_priv_size),
> + GFP_KERNEL);
> + if (!priv)
> + return ERR_PTR(-ENOMEM);
> + priv->destructor = family->sock_priv_destroy;
family->sock_priv_destroy may be in module memory.
I think you need to wipe them when family goes :(
> + if (family->sock_priv_init)
> + family->sock_priv_init(priv->priv);
> + return priv;
> +}
> +static struct xarray *genl_family_privs_get(struct genl_sock *gsk)
> +{
> + struct xarray *family_privs;
> +
> +again:
> + family_privs = READ_ONCE(gsk->family_privs);
> + if (family_privs)
> + return family_privs;
> +
> + family_privs = kzalloc(sizeof(*family_privs), GFP_KERNEL);
> + if (!family_privs)
> + return ERR_PTR(-ENOMEM);
> + xa_init_flags(family_privs, XA_FLAGS_ALLOC);
> +
> + /* Use genl lock to protect family_privs to be
> + * initialized in parallel by different CPU.
> + */
> + genl_lock();
> + if (unlikely(gsk->family_privs)) {
> + xa_destroy(family_privs);
> + kfree(family_privs);
> + genl_unlock();
nit: unlock can be moved up
> + goto again;
why not return READ_ONCE(gsk->family_privs); ?
there's no need to loop
One could also be tempted to:
lock()
if (likely(!gsk->family_privs)) {
WRITE
} else {
destory()
free()
family_privs = READ
}
unlock()
but it could be argued success path should be flat
> + }
> + WRITE_ONCE(gsk->family_privs, family_privs);
> + genl_unlock();
> + return family_privs;
> +}
next prev parent reply other threads:[~2023-12-08 2:55 UTC|newest]
Thread overview: 15+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-12-06 18:21 [patch net-next v5 0/9] devlink: introduce notifications filtering Jiri Pirko
2023-12-06 18:21 ` [patch net-next v5 1/9] devlink: use devl_is_registered() helper instead xa_get_mark() Jiri Pirko
2023-12-06 18:21 ` [patch net-next v5 2/9] devlink: introduce __devl_is_registered() helper and use it instead of xa_get_mark() Jiri Pirko
2023-12-06 18:21 ` [patch net-next v5 3/9] devlink: send notifications only if there are listeners Jiri Pirko
2023-12-06 18:21 ` [patch net-next v5 4/9] devlink: introduce a helper for netlink multicast send Jiri Pirko
2023-12-06 18:21 ` [patch net-next v5 5/9] genetlink: introduce per-sock family private storage Jiri Pirko
2023-12-08 2:55 ` Jakub Kicinski [this message]
2023-12-08 10:07 ` Jiri Pirko
2023-12-08 14:21 ` Jiri Pirko
2023-12-08 16:11 ` Jakub Kicinski
2023-12-09 10:36 ` Jiri Pirko
2023-12-06 18:21 ` [patch net-next v5 6/9] netlink: introduce typedef for filter function Jiri Pirko
2023-12-06 18:21 ` [patch net-next v5 7/9] genetlink: introduce helpers to do filtered multicast Jiri Pirko
2023-12-06 18:21 ` [patch net-next v5 8/9] devlink: add a command to set notification filter and use it for multicasts Jiri Pirko
2023-12-06 18:21 ` [patch net-next v5 9/9] devlink: extend multicast filtering by port index Jiri Pirko
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20231207185526.5e59ab53@kernel.org \
--to=kuba@kernel$(echo .)org \
--cc=amritha.nambiar@intel$(echo .)com \
--cc=andriy.shevchenko@linux$(echo .)intel.com \
--cc=davem@davemloft$(echo .)net \
--cc=edumazet@google$(echo .)com \
--cc=horms@kernel$(echo .)org \
--cc=jacob.e.keller@intel$(echo .)com \
--cc=jhs@mojatatu$(echo .)com \
--cc=jiri@resnulli$(echo .)us \
--cc=johannes@sipsolutions$(echo .)net \
--cc=netdev@vger$(echo .)kernel.org \
--cc=pabeni@redhat$(echo .)com \
--cc=przemyslaw.kitszel@intel$(echo .)com \
--cc=sdf@google$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox