Re: [PATCH net-next v11 12/23] ovpn: implement TCP transport

[Sergey Ryazanov <ryazanov.s.a@gmail.com>]

Hi Antonio,

the question was addressed to Sabrina, but since I've already touched 
this topic in the another patch, let me put my 2c here.

On 16.11.2024 02:33, Antonio Quartulli wrote:
> On 31/10/2024 16:25, Sabrina Dubroca wrote:
>> 2024-10-29, 11:47:25 +0100, Antonio Quartulli wrote:
>>> +static void ovpn_socket_release_work(struct work_struct *work)
>>> +{
>>> +    struct ovpn_socket *sock = container_of(work, struct 
>>> ovpn_socket, work);
>>> +
>>> +    ovpn_socket_detach(sock->sock);
>>> +    kfree_rcu(sock, rcu);
>>> +}
>>> +
>>> +static void ovpn_socket_schedule_release(struct ovpn_socket *sock)
>>> +{
>>> +    INIT_WORK(&sock->work, ovpn_socket_release_work);
>>> +    schedule_work(&sock->work);
>>
>> How does module unloading know that it has to wait for this work to
>> complete? Will ovpn_cleanup get stuck until some refcount gets
>> released by this work?
> 
> No, we have no such mechanism.
> Any idea how other modules do?
> 
> Actually this makes me wonder how module unloading coordinates with the 
> code being executed. Unload may happen at any time - how do we prevent 
> killing the code in the middle of something (regardless of scheduled 
> workers)?

Right question! There is a workqueue flushing API intended for 
synchronization with work(s) execution.

Here, the system workqueue was used, so technically a 
flush_scheduled_work() call somewhere in the module_exit handler would 
be enough.

On another hand, flushing the system workqueue considered a not so good 
practice. It's recommended to use a local workqueue. You can find a good 
example of switching from the system to a local workqueue in 
cc271ab86606 ("wwan_hwsim: Avoid flush_scheduled_work() usage").


And if the workqueue is definitely empty at a time of module unloading, 
e.g. due to flushing on netdev removing, there no requirement to flush 
it again.

--
Sergey