From: Longpeng <[email protected]>
The comment above the invocation of vfs_poll() is misleading, move
it to the right place.
Fixes: 684a0b719ddb ("KVM: eventfd: Fix lock order inversion")
Signed-off-by: Longpeng <[email protected]>
---
virt/kvm/eventfd.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/virt/kvm/eventfd.c b/virt/kvm/eventfd.c
index 2ad013b..cd01814 100644
--- a/virt/kvm/eventfd.c
+++ b/virt/kvm/eventfd.c
@@ -406,12 +406,12 @@ bool __attribute__((weak)) kvm_arch_irqfd_route_changed(
spin_unlock_irq(&kvm->irqfds.lock);
+ events = vfs_poll(f.file, &irqfd->pt);
+
/*
* Check if there was an event already pending on the eventfd
* before we registered, and trigger it as if we didn't miss it.
*/
- events = vfs_poll(f.file, &irqfd->pt);
-
if (events & EPOLLIN)
schedule_work(&irqfd->inject);
--
1.8.3.1
Am 29.11.21 um 04:43 schrieb Longpeng(Mike):
> From: Longpeng <[email protected]>
>
> The comment above the invocation of vfs_poll() is misleading, move
> it to the right place.
>
I think that the current variant is better.
events is only used in that function to check for EPOLLIN, so the
assignment and the if belong together from a "what am I doing here" perspective.
> Fixes: 684a0b719ddb ("KVM: eventfd: Fix lock order inversion")
> Signed-off-by: Longpeng <[email protected]>
> ---
> virt/kvm/eventfd.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/virt/kvm/eventfd.c b/virt/kvm/eventfd.c
> index 2ad013b..cd01814 100644
> --- a/virt/kvm/eventfd.c
> +++ b/virt/kvm/eventfd.c
> @@ -406,12 +406,12 @@ bool __attribute__((weak)) kvm_arch_irqfd_route_changed(
>
> spin_unlock_irq(&kvm->irqfds.lock);
>
> + events = vfs_poll(f.file, &irqfd->pt);
> +
> /*
> * Check if there was an event already pending on the eventfd
> * before we registered, and trigger it as if we didn't miss it.
> */
> - events = vfs_poll(f.file, &irqfd->pt);
> -
> if (events & EPOLLIN)
> schedule_work(&irqfd->inject);
>
> -----Original Message-----
> From: Christian Borntraeger [mailto:[email protected]]
> Sent: Tuesday, November 30, 2021 1:14 AM
> To: Longpeng (Mike, Cloud Infrastructure Service Product Dept.)
> <[email protected]>; [email protected]
> Cc: [email protected]; [email protected];
> [email protected]; Gonglei (Arei) <[email protected]>
> Subject: Re: [PATCH] kvm/eventfd: fix the misleading comment in kvm_irqfd_assign
>
>
>
> Am 29.11.21 um 04:43 schrieb Longpeng(Mike):
> > From: Longpeng <[email protected]>
> >
> > The comment above the invocation of vfs_poll() is misleading, move
> > it to the right place.
> >
> I think that the current variant is better.
> events is only used in that function to check for EPOLLIN, so the
> assignment and the if belong together from a "what am I doing here" perspective.
>
Hi Christian,
I think that add the irqfd->wait to the file's wait queue is much more
important, the current variant may lead to ignoring it.
Both of these two variants are supported in the current kernel:
[1] https://elixir.bootlin.com/linux/latest/source/drivers/vfio/virqfd.c#L169
```
events = vfs_poll(irqfd.file, &virqfd->pt);
/*
* Check if there was an event already pending on the eventfd
* before we registered and trigger it as if we didn't miss it.
*/
if (events & EPOLLIN) {
if ((!handler || handler(opaque, data)) && thread)
schedule_work(&virqfd->inject);
}
```
[2] https://elixir.bootlin.com/linux/latest/source/drivers/virt/acrn/irqfd.c#L161
```
/* Check the pending event in this stage */
events = vfs_poll(f.file, &irqfd->pt);
if (events & EPOLLIN)
acrn_irqfd_inject(irqfd);
```
Since there's no any code changes, I agree to drop this unmeaning change.
Thanks.
> > Fixes: 684a0b719ddb ("KVM: eventfd: Fix lock order inversion")
> > Signed-off-by: Longpeng <[email protected]>
> > ---
> > virt/kvm/eventfd.c | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/virt/kvm/eventfd.c b/virt/kvm/eventfd.c
> > index 2ad013b..cd01814 100644
> > --- a/virt/kvm/eventfd.c
> > +++ b/virt/kvm/eventfd.c
> > @@ -406,12 +406,12 @@ bool __attribute__((weak))
> kvm_arch_irqfd_route_changed(
> >
> > spin_unlock_irq(&kvm->irqfds.lock);
> >
> > + events = vfs_poll(f.file, &irqfd->pt);
> > +
> > /*
> > * Check if there was an event already pending on the eventfd
> > * before we registered, and trigger it as if we didn't miss it.
> > */
> > - events = vfs_poll(f.file, &irqfd->pt);
> > -
> > if (events & EPOLLIN)
> > schedule_work(&irqfd->inject);
> >