Le 26/08/2024 à 11:15, Przemek Kitszel a écrit :
On 8/23/24 11:10, Dan Carpenter wrote:
On Fri, Aug 23, 2024 at 08:23:29AM +0200, Christophe JAILLET wrote:
It would be even nicer to move the ma_list allocation outside the loop:
buf_size = struct_size(ma_list, mac_addr_list, IDPF_NUM_FILTERS_PER_MSG);
ma_list = kmalloc(buf_size, GFP_ATOMIC);
good point
I've opened whole function for inspection and it asks for even more,
as of now, we allocate an array in atomic context, just to have a copy
of some stuff from the spinlock-protected list.
It would be good to have allocation as pointed by Dan prior to iteration
and fill it on the fly, sending when new message would not fit plus once
at the end. Sending procedure is safe to be called under a spinlock.
If I understand correctly, you propose to remove the initial copy in mac_addr and hold &vport_config->mac_filter_list_lock till the end of the function?
That's it?
There is a wait_for_completion_timeout() in idpf_vc_xn_exec() and the default time-out is IDPF_VC_XN_DEFAULT_TIMEOUT_MSEC (60 * 1000)
So, should an issue occurs, and the time out run till the end, we could hold the 'mac_filter_list_lock' spinlock for up to 60 seconds?
Is that ok?
And if in asynch update mode, idpf_mac_filter_async_handler() also takes &vport_config->mac_filter_list_lock;. Could we dead-lock?
So, I'm not sure to understand what you propose, or the code in idpf_add_del_mac_filters() and co.
CCing author; CCing Olek to ask if there are already some refactors that
would conflict with this.
I'll way a few days for these feedbacks and send a v2.