Re: [patch net-next v4 5/9] genetlink: introduce per-sock family private pointer storage

[Jiri Pirko <jiri@resnulli.us>]

Tue, Nov 28, 2023 at 05:36:05PM CET, kuba@kernel.org wrote:
>On Tue, 28 Nov 2023 17:05:48 +0100 Jiri Pirko wrote:
>> >Not necessarily, you can have a helper which doesn't allocate, too.
>> >What I'm saying is that the common case for ops will be to access
>> >the state and allocate if it doesn't exist.
>> >
>> >How about genl_sk_family_priv() and genl_sk_has_family_priv() ?  
>> 
>> My point is, with what you suggest, it will look something like this:
>> 
>> 1) user does DEVLINK_CMD_NOTIFY_FILTER_SET
>> 2) devlink calls into genetlink code for a sk_priv and inserts in xa_array
>> 3) genetlink allocates sk_priv and returns back
>> 4) devlink fills-up the sk_priv
>> 
>> 5) user does DEVLINK_CMD_NOTIFY_FILTER_SET, again
>> 6) devlink calls into genetlink code for a sk_priv
>> 7) genetlink returns already exising sk_priv found in xa_array
>> 8) devlink fills-up the sk_priv
>> 
>> Now the notification thread, sees sk_priv zeroed between 3) and 4)
>> and inconsistent during 4) and 8)
>> 
>> I originally solved that by rcu, DEVLINK_CMD_NOTIFY_FILTER_SET
>> code always allocates and flips the rcu pointer. Notification thread
>> always sees sk_priv consistent.
>> 
>> If you want to allocate sk_priv in genetlink code once, hard to use
>> the rcu mechanism and have to protect the sk_priv memory by a lock.
>
>No, you can do exact same thing, just instead of putting the string
>directly into the xarray you put a struct which points to the string.

I'm lost. What "string" are you talking about exactly? I'm not putting
any string to xarray.

In the existing implementation, I have following struct:
struct devlink_obj_desc {
        struct rcu_head rcu;
        const char *bus_name;
        const char *dev_name;
        unsigned int port_index;
        bool port_index_valid;
        long data[];
};

This is the struct put pointer to into the xarray. Pointer to this
struct is dereferenced under rcu in notification code and the struct
is freed by kfree_rcu().


>
>> What am I missing?
>
>The fact that someone in the future may want to add another devlink
>priv field, and if the state is basically a pointer to a string,

"the state is basically a pointer to a string". I don't follow what you
mean by this :/


>with complicated lifetime, they will have to suffer undoing that.
>
>> >> If it is alloceted automatically, why is it needed?  
>> >
>> >Because priv may be a complex type which has member that need
>> >individual fields to be destroyed (in fullness of time we also
>> >need a constructor which can init things like list_head, but
>> >we can defer that).
>> >
>> >I'm guessing in your case the priv will look like this:
>> >
>> >struct devlink_sk_priv {
>> >	const char *nft_fltr_instance_name;
>> >};
>> >
>> >static void devlink_sk_priv_free(void *ptr)
>> >{
>> >	struct devlink_sk_priv *priv = ptr;
>> >
>> >	kfree(priv->nft_fltr_instance_name);
>> >}  
>> 
>> If genetlink code does the allocation, it should know how to free.
>> Does not make sense to pass destructor to genetlink code to free memory
>> it actually allocated :/
>> 
>> If devlink does the allocation, this callback makes sense. I was
>> thinking about having it, but decided kfree is okay for now and
>> destructor could be always introduced if needed.
>
>Did you read the code snippet above?

Sure.


>
>Core still does the kfree of the container (struct devlink_sk_priv).
>But what's inside the container struct (string pointer) has to be
>handled by the destructor.
>
>Feels like you focus on how to prove me wrong more than on
>understanding what I'm saying :|

Not at all, I have no reason for it. I just want to get my job done
and I am having very hard time to understand what you want exactly.