On 2/13/24 9:46 AM, Leon Romanovsky wrote:
> On Tue, Feb 13, 2024 at 09:59:54AM -0600, Bjorn Helgaas wrote:
>> On Tue, Feb 13, 2024 at 09:34:50AM +0200, Leon Romanovsky wrote:
>>> On Mon, Feb 12, 2024 at 02:27:14PM -0600, Bjorn Helgaas wrote:
>>>> On Sun, Feb 11, 2024 at 10:48:44AM +0200, Leon Romanovsky wrote:
>>>>> On Fri, Feb 09, 2024 at 07:20:28PM -0800, Kuppuswamy Sathyanarayanan wrote:
>>>>>> On 2/9/24 3:52 PM, Jim Harris wrote:
>>>>>>> If an SR-IOV enabled device is held by vfio, and the device
>>>>>>> is removed, vfio will hold device lock and notify userspace
>>>>>>> of the removal. If userspace reads the sriov_numvfs sysfs
>>>>>>> entry, that thread will be blocked since sriov_numvfs_show()
>>>>>>> also tries to acquire the device lock. If that same thread
>>>>>>> is responsible for releasing the device to vfio, it results
>>>>>>> in a deadlock.
>>>>>>>
>>>>>>> The proper way to detect a change to the num_VFs value is to
>>>>>>> listen for a sysfs event, not to add a device_lock() on the
>>>>>>> attribute _show() in the kernel.
>>>> The lock was not about detecting a change; Pierre did this:
>>>>
>>>> ip monitor dev ${DEVICE} | grep --line-buffered "^${id}:" | while read line; do \
>>>> cat ${path}/device/sriov_numvfs; \
>>>>
>>>> which I assume works by listening for sysfs events.
>>> It is not, "ip monitor ..." listens to netlink events emitted by
>>> netdev core and not sysfs events. Sysfs events are not involved in
>>> this case.
>> Thanks for correcting my hasty assumption!
>>
>>>> The problem was that after the event occurred, the sriov_numvfs
>>>> read got a stale value (see https://bugzilla.kernel.org/show_bug.cgi?id=202991).
>>> Yes, and it is outcome of such cross-subsytem involvement, which
>>> is racy by definition. Someone can come with even simpler example of why
>>> locking sysfs read and write is not a good idea.
>>>
>>> For example, let's consider the following scenario with two CPUs and
>>> locks on sysfs read and write:
>>>
>>> CPU1 CPU2
>>> echo 1 > ${path}/device/sriov_numvfs
>>> context_switch ->
>>> cat ${path}/device/sriov_numvfs
>>> lock
>>> return 0
>>> unlock
>>> context_switch <-
>>> lock
>>> set 1
>>> unlock
>>>
>>> CPU1 CPU2
>>> echo 1 > ${path}/device/sriov_numvfs
>>> lock
>>> set 1
>>> unlock
>>> context_switch ->
>>> cat ${path}/device/sriov_numvfs
>>> lock
>>> return 1
>>> unlock
>>>
>>> So same scenario will return different values if user doesn't protect
>>> such case with external to the kernel lock.
>>>
>>> But if we return back to Pierre report and if you want to provide
>>> completely bullet proof solution to solve cross-subsystem interaction,
>>> you will need to prohibit device probe till sriov_numvfs update is completed.
>>> However, it is overkill for something that is not a real issue.
>> Pierre wanted to detect the configuration change and learn the new
>> num_vfs, which seems like a reasonable thing to do. Is there a way to
>> do both via netlink or some other mechanism?
> Please pay attention that Pierre listened to specific netdevice and not
> to something general. After patch #2 in Jim's series, he will be able to
> rely on "udevadm monitor" instead of "ip monitor".
>
>>>> So I would drop this sentence because I don't think it accurately
>>>> reflects the reason for 35ff867b7657.
>>>>
>>>>>> Since you are reverting a commit that synchronizes SysFS read
>>>>>> /write, please add some comments about why it is not an
>>>>>> issue anymore.
>>>>> It was never an issue, the idea that sysfs read and write should be
>>>>> serialized by kernel is not correct by definition.
>>>> I think it *was* an issue. The behavior Pierre observed at was
>>>> clearly wrong,
>>> I disagree with this sentence.
>>>
>>>> and we added 35ff867b7657 ("PCI/IOV: Serialize sysfs
>>>> sriov_numvfs reads vs writes") to resolve it.
>>>>
>>>> We should try to avoid reintroducing the problem, so I think we should
>>>> probably squash these two patches and describe it as a deadlock fix
>>>> instead of dismissing 35ff867b7657 as being based on false premises.
>>>>
>>>> It would be awesome if you had time to verify that these patches also
>>>> resolve the problem you saw, Pierre.
>>> They won't resolve his problem, because he is not listening to sysfs
>>> events, but rely on something from netdev side.
>> I guess that means that if we apply this revert, the problem Pierre
>> reported will return. Obviously the deadlock is more important than
>> the inconsistency Pierre observed, but from the user's point of view
>> this will look like a regression.
>>
>> Maybe listening to netlink and then looking at sysfs isn't the
>> "correct" way to do this, but I don't want to just casually break
>> existing user code. If we do contemplate doing the revert, at the
>> very least we should include specific details about what the user code
>> *should* do instead, at the level of the actual commands to use
>> instead of "ip monitor dev; cat ${path}/device/sriov_numvfs".
> udevadm monitor will do the trick.
>
> Another possible solution is to refactor the code to make sure that
> .probe on VFs happens only after sriov_numvfs is updated.
>
> Thanks
IMO, we can update the sriov_numvfs documentation to let users
aware of the possible race condition between read/write, and also
suggestion about using uevents for device changes.
>> Bjorn
>>
--
Sathyanarayanan Kuppuswamy
Linux Kernel Developer
