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

[Jiri Pirko <jiri@resnulli.us>]

Fri, Oct 06, 2023 at 04:48:42PM CEST, kuba@kernel.org wrote:
>On Fri, 6 Oct 2023 09:22:01 +0200 Jiri Pirko wrote:
>> Fri, Oct 06, 2023 at 03:30:29AM CEST, kuba@kernel.org wrote:
>> >> @@ -310,6 +299,7 @@ static void devlink_release(struct work_struct *work)
>> >>  
>> >>  	mutex_destroy(&devlink->lock);
>> >>  	lockdep_unregister_key(&devlink->lock_key);
>> >> +	put_device(devlink->dev);  
>> >
>> >IDK.. holding references until all references are gone may lead 
>> >to reference cycles :(  
>> 
>> I don't follow. What seems to be the problematic flow? I can't spot any
>> reference cycle, do you?
>
>I can't remember to be honest. But we already assume that we can access
>struct device of a devlink instance without holding the instance lock.
>Because the relationship between devlink objects is usually fairly
>straightforward and non-cyclical.
>
>Isn't the "rel infrastructure"... well.. over-designed?
>
>The user creates a port on an instance A, which spawns instance B.
>Instance A links instance B to itself.
>Instance A cannot disappear before instance B disappears.

It can. mlx5 port sf removal is very nice example of that. It just tells
the FW to remove the sf and returns. The actual SF removal is spawned
after that when processing FW events.


>Also instance A is what controls the destruction of instance B
>so it can unlink it.
>
>We can tell lockdep how the locks nest, too.
>
>> >Overall I feel like recording the references on the objects will be
>> >an endless source of locking pain. Would it be insane if we held 
>> >the relationships as independent objects? Not as attributes of either
>> >side?   
>> 
>> How exactly do you envision this? rel struct would hold the bus/name
>> strings direcly?
>
>No exactly, if we want bi-directional relationships we can create 
>the link struct as a:
>
>rel {
>	u32 rel_id;
>	struct devlink *instanceA, *instanceB; // hold reference

Sometimes port, sometimes linecard is one one side (A).


>	struct list_head rel_listA, rel_listB; // under instance locks
>	u32 state;
>	struct list_head ntf_process_queue;
>}
>
>Operations on relationship can take the instance locks (sequentially).
>Notifications from a workqueue.

That is pretty much how that works now.


>Instance dumps would only report rel IDs, but the get the "members" of
>the relationship user needs to issue a separate DL command / syscall.

Oh! At least with process listening on notifications, this may be a bit
painful :/
I need some time to digest this.