Re: [patch net-next] devlink: don't take instance lock for nested handle put

Jiri Pirko
2023-10-11T13:35:05Z

Tue, Oct 10, 2023 at 08:16:05PM CEST, kuba@kernel.org wrote:
>On Tue, 10 Oct 2023 17:56:36 +0200 Jiri Pirko wrote:
>> >You understand what I'm saying tho, right?
>> >
>> >If we can depend on the parent not disappearing before the child,
>> >and the hierarchy is a DAG - the locking is much easier, because
>> >parent can lock the child.
>>
>> It won't help with the locking though. During GET, the devlink lock
>> is taken and within it, you need to access the nested devlink attributes.
>>
>> And during reload->notify, we still need work so the lock are taken in
>> proper order.
>
>If parent is guaranteed to exist the read only fields can be accessed
>freely and the read-write fields can be cached on children.

Only reason to access parent currently is netns change notification.
See devlink_rel_nested_in_notify().
It basically just scheduled delayed work by calling:
devlink_rel_nested_in_notify_work_schedule().

When work is processed in
devlink_rel_nested_in_notify_work()
There is no guarantee the parent exists, therefore devlink_index is used
to get the instance and then obj_index to get port/linecard index.

notify_cb() basically sends notification of parent object and that needs
parent instance lock. <--- This is why you need to lock the parent.

I see no way how to cache anything on children as you describe in this
scenario.

>Parent has a list of children, it can store/cache a netns pointer on all
>of them. When reload happens lock them and update that pointer.
>At which point children do not have to lock the parent.

Access of netns pointer is not a problem. See my latest version (v2)
where rcu is used in order to make sure peernet2id_alloc() call is safe:

devlink: call peernet2id_alloc() with net pointer under RCU read lock

rcu_read_lock();
devl_net = read_pnet_rcu(&devlink->_net);
if (!net_eq(net, devl_net)) {
int id = peernet2id_alloc(net, devl_net, GFP_ATOMIC);

rcu_read_unlock();
if (nla_put_s32(msg, DEVLINK_ATTR_NETNS_ID, id))
return -EMSGSIZE;
} else {
rcu_read_unlock();
}

>
>> It would only make the rel infrastructure a bit similer. I will look
>> into that. But it's parallel to this patchset really.
>