* [PATCH] kvm/eventfd: fix the misleading comment in kvm_irqfd_assign
@ 2021-11-29 3:43 Longpeng(Mike)
2021-11-29 17:13 ` Christian Borntraeger
0 siblings, 1 reply; 3+ messages in thread
From: Longpeng(Mike) @ 2021-11-29 3:43 UTC (permalink / raw)
To: pbonzini; +Cc: cornelia.huck, kvm, linux-kernel, arei.gonglei, Longpeng
From: Longpeng <longpeng2@huawei.com>
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 <longpeng2@huawei.com>
---
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
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] kvm/eventfd: fix the misleading comment in kvm_irqfd_assign
2021-11-29 3:43 [PATCH] kvm/eventfd: fix the misleading comment in kvm_irqfd_assign Longpeng(Mike)
@ 2021-11-29 17:13 ` Christian Borntraeger
2021-12-03 3:03 ` Longpeng (Mike, Cloud Infrastructure Service Product Dept.)
0 siblings, 1 reply; 3+ messages in thread
From: Christian Borntraeger @ 2021-11-29 17:13 UTC (permalink / raw)
To: Longpeng(Mike), pbonzini; +Cc: cornelia.huck, kvm, linux-kernel, arei.gonglei
Am 29.11.21 um 04:43 schrieb Longpeng(Mike):
> From: Longpeng <longpeng2@huawei.com>
>
> 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 <longpeng2@huawei.com>
> ---
> 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);
>
^ permalink raw reply [flat|nested] 3+ messages in thread
* RE: [PATCH] kvm/eventfd: fix the misleading comment in kvm_irqfd_assign
2021-11-29 17:13 ` Christian Borntraeger
@ 2021-12-03 3:03 ` Longpeng (Mike, Cloud Infrastructure Service Product Dept.)
0 siblings, 0 replies; 3+ messages in thread
From: Longpeng (Mike, Cloud Infrastructure Service Product Dept.) @ 2021-12-03 3:03 UTC (permalink / raw)
To: Christian Borntraeger, pbonzini
Cc: cornelia.huck, kvm, linux-kernel, Gonglei (Arei)
> -----Original Message-----
> From: Christian Borntraeger [mailto:borntraeger@linux.ibm.com]
> Sent: Tuesday, November 30, 2021 1:14 AM
> To: Longpeng (Mike, Cloud Infrastructure Service Product Dept.)
> <longpeng2@huawei.com>; pbonzini@redhat.com
> Cc: cornelia.huck@de.ibm.com; kvm@vger.kernel.org;
> linux-kernel@vger.kernel.org; Gonglei (Arei) <arei.gonglei@huawei.com>
> 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 <longpeng2@huawei.com>
> >
> > 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 <longpeng2@huawei.com>
> > ---
> > 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);
> >
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2021-12-03 3:03 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-11-29 3:43 [PATCH] kvm/eventfd: fix the misleading comment in kvm_irqfd_assign Longpeng(Mike)
2021-11-29 17:13 ` Christian Borntraeger
2021-12-03 3:03 ` Longpeng (Mike, Cloud Infrastructure Service Product Dept.)
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).