lkml.org 
[lkml]   [2021]   [Jun]   [30]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH] s390/vfio-ap: do not use open locks during VFIO_GROUP_NOTIFY_SET_KVM notification
On Wed, 30 Jun 2021 10:31:22 -0400
Tony Krowiak <akrowiak@linux.ibm.com> wrote:

> On 6/28/21 4:29 PM, Halil Pasic wrote:
> > On Fri, 25 Jun 2021 18:07:58 -0400
> > Tony Krowiak <akrowiak@linux.ibm.com> wrote:
> >
> > What is a suitable base for this patch. I've tried the usual suspects,
> > but none of them worked.
>
> I discovered what the problem is here. The patch is based on our
> master branch along with the two pre-requisite patches that were
> recently reviewed and are currently being merged. The two patches
> of which I speak are:
> * [PATCH v6 1/2] s390/vfio-ap: clean up mdev resources when remove
> callback invoked
>    Message ID: <20210621155714.1198545-2-akrowiak@linux.ibm.com>
>
> * [PATCH v6 2/2] s390/vfio-ap: r/w lock for PQAP interception handler
> function pointer
>    <20210621155714.1198545-3-akrowiak@linux.ibm.com>
>
> I probably should have included those along with this one.

Either that, or state in the cover letter that those are prerequisites.

>
> >
> >> The fix to resolve a lockdep splat while handling the
> >> VFIO_GROUP_NOTIFY_SET_KVM event introduced a kvm_busy flag indicating that
> >> the vfio_ap device driver is busy setting or unsetting the KVM pointer.
> >> A wait queue was employed to allow functions requiring access to the KVM
> >> pointer to wait for the kvm_busy flag to be cleared. For the duration of
> >> the wait period, the mdev lock was unlocked then acquired again after the
> >> kvm_busy flag was cleared. This got rid of the lockdep report, but didn't
> >> really resolve the problem.
> > Can you please elaborate on the last point. You mean that we can have
> > circular locking even after 0cc00c8d4050, but instead of getting stuck in
> > on a lock we will get stuck on wait_event_cmd()? If that is it, please
> > state it clearly in the description, and if you can to it in the short
> > description.
>
> This patch was in response to the following review comments made by Jason
> Gunthorpe:
>
> * Message ID: <20210525162927.GC1002214@nvidia.com>
>    "... the kvm_busy should be replaced by a proper rwsem,
>     don't try to open code locks like that - it just defeats lockdep
>     analysis".
>
> * Message ID: <20210527112433.GX1002214@nvidia.com>
>    "Usually when people start open coding locks it is often
>    because lockdep complained. Open coding a lock makes
>    lockdep stop because the lockdep code
>    is removed, but it doesn't fix anything. The kvm_busy
>    should be replaced by a proper rwsem, don't try to
>    open code locks like that - it just defeats lockdep
>    analysis."
>
> I will paraphrase and include the information from Jason's
> comments in the description.
>

This does not answer my questions.

I'm in favor of Jason's proposal, because it is much easier to
comprehend simple rwsem protected than a mutex + wait_queue dance.

I think Jason was talking about open coding locks in general. I don't
consider it as proof of commit 0cc00c8d4050 not doing what it
advertised. You can add a Suggested-by tag if you like, but you should
be able to tell us what is the merit of your patch.

Regards,
Halil

\
 
 \ /
  Last update: 2021-07-01 00:40    [W:2.577 / U:0.844 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site