Re: [PATCH] PCI/IOV: update num_VFs earlier

[CREGUT Pierre IMT/OLN]

Le 02/10/2019 Ã 01:45, Bjorn Helgaas a ÃcritÂ:
> On Fri, Apr 26, 2019 at 10:11:54AM +0200, CREGUT Pierre IMT/OLN wrote:
> > I also initially thought that kobject_uevent generated the netlink event
> > but this is not the case. This is generated by the specific driver in use.
> > For the Intel i40e driver, this is the call to i40e_do_reset_safe in
> > i40e_pci_sriov_configure that sends the event.
> > It is followed by i40e_pci_sriov_enable that calls i40e_alloc_vfs that
> > finally calls the generic pci_enable_sriov function.
> I don't know anything about netlink.  The script from the bugzilla
> (https://bugzilla.kernel.org/show_bug.cgi?id=202991) looks like it
> runs
>
>    ip monitor dev enp9s0f2
>
> What are the actual netlink events you see?  Are they related to a
> device being removed?

We have netlink events both when num_vfs goes from 0 to N and from N to 0.
Indeed you have to go to 0 before going to M with M != N.
On an Intel card, when one goes from 0 to N, the netlink event is sent 
"early". The
value of num_vfs is still 0 and you get the impression that the number 
of VFS has
not changed. As the meaning of those events is overloaded, you have to 
wait an
arbitrary amount of time until it settles (there will be no other event).
There is no such problem when it goes from N to 0 because of 
implementation details
but it may be different for another brand.

> When we change num_VFs, I think we have to disable any existing VFs
> before enabling the new num_VFs, so if you trigger on a netlink
> "remove" event, I wouldn't be surprised that reading sriov_numvfs
> would give a zero until the new VFs are enabled.
Yes but we are speaking of the event sent when num_vfs is changed from 0 
to N
> [...]
> I thought this was a good idea, but
>
>    - It does break the device_lock() encapsulation a little bit:
>      sriov_numvfs_store() uses device_lock(), which happens to be
>      implemented as "mutex_lock(&dev->mutex)", but we really shouldn't
>      rely on that implementation, and
The use of device_lock was the cheapest solution. It is true that lock 
and trylock are
exposed by device.h but not is_locked. To respect the abstraction, we 
would have to
lock the device (at least use trylock but it means locking when we can 
access the
value, in that case we may just make reading num_vfs blocking ?).

The other solution is to record the state of freshness of num_vfs but it 
means a
new Boolean in the pci_sriov data-structure.

>    - The netlink events are being generated via the NIC driver, and I'm
>      a little hesitant about changing the PCI core to deal with timing
>      issues "over there".

NIC drivers send netlink events when their state change, but it is the 
core that changes
the value of num_vfs. So I would think it is the core responsibility to 
make sure the
exposed value makes sense and it would be better to ignore the details 
of the driver
implementation.
That is why the initial patch moving when the value was updated was 
finally not
such a good idea.

[...]