* [PATCH] vfio/platform: Replace spin_lock_irqsave by spin_lock in hard IRQ
@ 2020-10-13 2:00 Tian Tao
2020-10-13 21:32 ` Alex Williamson
0 siblings, 1 reply; 5+ messages in thread
From: Tian Tao @ 2020-10-13 2:00 UTC (permalink / raw)
To: eric.auger, alex.williamson, cohuck, kvm, linux-kernel, song.bao.hua
Cc: linuxarm
It is redundant to do irqsave and irqrestore in hardIRQ context.
Signed-off-by: Tian Tao <tiantao6@hisilicon.com>
---
drivers/vfio/platform/vfio_platform_irq.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/drivers/vfio/platform/vfio_platform_irq.c b/drivers/vfio/platform/vfio_platform_irq.c
index c5b09ec..24fd6c5 100644
--- a/drivers/vfio/platform/vfio_platform_irq.c
+++ b/drivers/vfio/platform/vfio_platform_irq.c
@@ -139,10 +139,9 @@ static int vfio_platform_set_irq_unmask(struct vfio_platform_device *vdev,
static irqreturn_t vfio_automasked_irq_handler(int irq, void *dev_id)
{
struct vfio_platform_irq *irq_ctx = dev_id;
- unsigned long flags;
int ret = IRQ_NONE;
- spin_lock_irqsave(&irq_ctx->lock, flags);
+ spin_lock(&irq_ctx->lock);
if (!irq_ctx->masked) {
ret = IRQ_HANDLED;
@@ -152,7 +151,7 @@ static irqreturn_t vfio_automasked_irq_handler(int irq, void *dev_id)
irq_ctx->masked = true;
}
- spin_unlock_irqrestore(&irq_ctx->lock, flags);
+ spin_unlock(&irq_ctx->lock);
if (ret == IRQ_HANDLED)
eventfd_signal(irq_ctx->trigger, 1);
--
2.7.4
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] vfio/platform: Replace spin_lock_irqsave by spin_lock in hard IRQ
2020-10-13 2:00 [PATCH] vfio/platform: Replace spin_lock_irqsave by spin_lock in hard IRQ Tian Tao
@ 2020-10-13 21:32 ` Alex Williamson
2020-10-14 0:15 ` Song Bao Hua (Barry Song)
0 siblings, 1 reply; 5+ messages in thread
From: Alex Williamson @ 2020-10-13 21:32 UTC (permalink / raw)
To: Tian Tao; +Cc: eric.auger, cohuck, kvm, linux-kernel, song.bao.hua, linuxarm
On Tue, 13 Oct 2020 10:00:58 +0800
Tian Tao <tiantao6@hisilicon.com> wrote:
> It is redundant to do irqsave and irqrestore in hardIRQ context.
But this function is also called from non-IRQ context. Thanks,
Alex
> Signed-off-by: Tian Tao <tiantao6@hisilicon.com>
> ---
> drivers/vfio/platform/vfio_platform_irq.c | 5 ++---
> 1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/vfio/platform/vfio_platform_irq.c b/drivers/vfio/platform/vfio_platform_irq.c
> index c5b09ec..24fd6c5 100644
> --- a/drivers/vfio/platform/vfio_platform_irq.c
> +++ b/drivers/vfio/platform/vfio_platform_irq.c
> @@ -139,10 +139,9 @@ static int vfio_platform_set_irq_unmask(struct vfio_platform_device *vdev,
> static irqreturn_t vfio_automasked_irq_handler(int irq, void *dev_id)
> {
> struct vfio_platform_irq *irq_ctx = dev_id;
> - unsigned long flags;
> int ret = IRQ_NONE;
>
> - spin_lock_irqsave(&irq_ctx->lock, flags);
> + spin_lock(&irq_ctx->lock);
>
> if (!irq_ctx->masked) {
> ret = IRQ_HANDLED;
> @@ -152,7 +151,7 @@ static irqreturn_t vfio_automasked_irq_handler(int irq, void *dev_id)
> irq_ctx->masked = true;
> }
>
> - spin_unlock_irqrestore(&irq_ctx->lock, flags);
> + spin_unlock(&irq_ctx->lock);
>
> if (ret == IRQ_HANDLED)
> eventfd_signal(irq_ctx->trigger, 1);
^ permalink raw reply [flat|nested] 5+ messages in thread
* RE: [PATCH] vfio/platform: Replace spin_lock_irqsave by spin_lock in hard IRQ
2020-10-13 21:32 ` Alex Williamson
@ 2020-10-14 0:15 ` Song Bao Hua (Barry Song)
2020-10-14 0:50 ` Alex Williamson
0 siblings, 1 reply; 5+ messages in thread
From: Song Bao Hua (Barry Song) @ 2020-10-14 0:15 UTC (permalink / raw)
To: Alex Williamson, tiantao (H)
Cc: eric.auger, cohuck, kvm, linux-kernel, Linuxarm
> -----Original Message-----
> From: Alex Williamson [mailto:alex.williamson@redhat.com]
> Sent: Wednesday, October 14, 2020 10:32 AM
> To: tiantao (H) <tiantao6@hisilicon.com>
> Cc: eric.auger@redhat.com; cohuck@redhat.com; kvm@vger.kernel.org;
> linux-kernel@vger.kernel.org; Song Bao Hua (Barry Song)
> <song.bao.hua@hisilicon.com>; Linuxarm <linuxarm@huawei.com>
> Subject: Re: [PATCH] vfio/platform: Replace spin_lock_irqsave by spin_lock in
> hard IRQ
>
> On Tue, 13 Oct 2020 10:00:58 +0800
> Tian Tao <tiantao6@hisilicon.com> wrote:
>
> > It is redundant to do irqsave and irqrestore in hardIRQ context.
>
> But this function is also called from non-IRQ context. Thanks,
It seems you mean
vfio_platform_set_irqs_ioctl() ->
vfio_platform_set_irq_trigger ->
handler() ?
so, will it be better to move the irqsave out of the vfio_automasked_irq_handler()
and put it to where the function is called in non-IRQ context?
I mean:
irqhandler()
{
spin_lock() //without irqsave
spin_unlock()
}
Non-irq context which is calling this handler:
irqsave();
irqhandler();
irqrestore();
Anyway, if it is called in IRQ context, it is redundant to do irqsave.
>
> Alex
>
> > Signed-off-by: Tian Tao <tiantao6@hisilicon.com>
> > ---
> > drivers/vfio/platform/vfio_platform_irq.c | 5 ++---
> > 1 file changed, 2 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/vfio/platform/vfio_platform_irq.c
> b/drivers/vfio/platform/vfio_platform_irq.c
> > index c5b09ec..24fd6c5 100644
> > --- a/drivers/vfio/platform/vfio_platform_irq.c
> > +++ b/drivers/vfio/platform/vfio_platform_irq.c
> > @@ -139,10 +139,9 @@ static int vfio_platform_set_irq_unmask(struct
> vfio_platform_device *vdev,
> > static irqreturn_t vfio_automasked_irq_handler(int irq, void *dev_id)
> > {
> > struct vfio_platform_irq *irq_ctx = dev_id;
> > - unsigned long flags;
> > int ret = IRQ_NONE;
> >
> > - spin_lock_irqsave(&irq_ctx->lock, flags);
> > + spin_lock(&irq_ctx->lock);
> >
> > if (!irq_ctx->masked) {
> > ret = IRQ_HANDLED;
> > @@ -152,7 +151,7 @@ static irqreturn_t vfio_automasked_irq_handler(int
> irq, void *dev_id)
> > irq_ctx->masked = true;
> > }
> >
> > - spin_unlock_irqrestore(&irq_ctx->lock, flags);
> > + spin_unlock(&irq_ctx->lock);
> >
> > if (ret == IRQ_HANDLED)
> > eventfd_signal(irq_ctx->trigger, 1);
Thanks
Barry
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] vfio/platform: Replace spin_lock_irqsave by spin_lock in hard IRQ
2020-10-14 0:15 ` Song Bao Hua (Barry Song)
@ 2020-10-14 0:50 ` Alex Williamson
2020-10-14 1:19 ` Song Bao Hua (Barry Song)
0 siblings, 1 reply; 5+ messages in thread
From: Alex Williamson @ 2020-10-14 0:50 UTC (permalink / raw)
To: Song Bao Hua (Barry Song)
Cc: tiantao (H), eric.auger, cohuck, kvm, linux-kernel, Linuxarm
On Wed, 14 Oct 2020 00:15:13 +0000
"Song Bao Hua (Barry Song)" <song.bao.hua@hisilicon.com> wrote:
> > -----Original Message-----
> > From: Alex Williamson [mailto:alex.williamson@redhat.com]
> > Sent: Wednesday, October 14, 2020 10:32 AM
> > To: tiantao (H) <tiantao6@hisilicon.com>
> > Cc: eric.auger@redhat.com; cohuck@redhat.com; kvm@vger.kernel.org;
> > linux-kernel@vger.kernel.org; Song Bao Hua (Barry Song)
> > <song.bao.hua@hisilicon.com>; Linuxarm <linuxarm@huawei.com>
> > Subject: Re: [PATCH] vfio/platform: Replace spin_lock_irqsave by spin_lock in
> > hard IRQ
> >
> > On Tue, 13 Oct 2020 10:00:58 +0800
> > Tian Tao <tiantao6@hisilicon.com> wrote:
> >
> > > It is redundant to do irqsave and irqrestore in hardIRQ context.
> >
> > But this function is also called from non-IRQ context. Thanks,
>
> It seems you mean
> vfio_platform_set_irqs_ioctl() ->
> vfio_platform_set_irq_trigger ->
> handler() ?
Yes.
> so, will it be better to move the irqsave out of the vfio_automasked_irq_handler()
> and put it to where the function is called in non-IRQ context?
>
> I mean:
>
> irqhandler()
> {
> spin_lock() //without irqsave
> spin_unlock()
> }
>
> Non-irq context which is calling this handler:
> irqsave();
> irqhandler();
> irqrestore();
>
> Anyway, if it is called in IRQ context, it is redundant to do irqsave.
What's the advantage? You're saying it's redundant, is it also wrong?
If it's not wrong and only redundant, what's the tangible latency
difference in maintaining a separate IRQ context handler without the
irqsave/restore? Thanks,
Alex
> > > Signed-off-by: Tian Tao <tiantao6@hisilicon.com>
> > > ---
> > > drivers/vfio/platform/vfio_platform_irq.c | 5 ++---
> > > 1 file changed, 2 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/vfio/platform/vfio_platform_irq.c
> > b/drivers/vfio/platform/vfio_platform_irq.c
> > > index c5b09ec..24fd6c5 100644
> > > --- a/drivers/vfio/platform/vfio_platform_irq.c
> > > +++ b/drivers/vfio/platform/vfio_platform_irq.c
> > > @@ -139,10 +139,9 @@ static int vfio_platform_set_irq_unmask(struct
> > vfio_platform_device *vdev,
> > > static irqreturn_t vfio_automasked_irq_handler(int irq, void *dev_id)
> > > {
> > > struct vfio_platform_irq *irq_ctx = dev_id;
> > > - unsigned long flags;
> > > int ret = IRQ_NONE;
> > >
> > > - spin_lock_irqsave(&irq_ctx->lock, flags);
> > > + spin_lock(&irq_ctx->lock);
> > >
> > > if (!irq_ctx->masked) {
> > > ret = IRQ_HANDLED;
> > > @@ -152,7 +151,7 @@ static irqreturn_t vfio_automasked_irq_handler(int
> > irq, void *dev_id)
> > > irq_ctx->masked = true;
> > > }
> > >
> > > - spin_unlock_irqrestore(&irq_ctx->lock, flags);
> > > + spin_unlock(&irq_ctx->lock);
> > >
> > > if (ret == IRQ_HANDLED)
> > > eventfd_signal(irq_ctx->trigger, 1);
>
> Thanks
> Barry
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* RE: [PATCH] vfio/platform: Replace spin_lock_irqsave by spin_lock in hard IRQ
2020-10-14 0:50 ` Alex Williamson
@ 2020-10-14 1:19 ` Song Bao Hua (Barry Song)
0 siblings, 0 replies; 5+ messages in thread
From: Song Bao Hua (Barry Song) @ 2020-10-14 1:19 UTC (permalink / raw)
To: Alex Williamson
Cc: tiantao (H), eric.auger, cohuck, kvm, linux-kernel, Linuxarm
> -----Original Message-----
> From: Alex Williamson [mailto:alex.williamson@redhat.com]
> Sent: Wednesday, October 14, 2020 1:50 PM
> To: Song Bao Hua (Barry Song) <song.bao.hua@hisilicon.com>
> Cc: tiantao (H) <tiantao6@hisilicon.com>; eric.auger@redhat.com;
> cohuck@redhat.com; kvm@vger.kernel.org; linux-kernel@vger.kernel.org;
> Linuxarm <linuxarm@huawei.com>
> Subject: Re: [PATCH] vfio/platform: Replace spin_lock_irqsave by spin_lock in
> hard IRQ
>
> On Wed, 14 Oct 2020 00:15:13 +0000
> "Song Bao Hua (Barry Song)" <song.bao.hua@hisilicon.com> wrote:
>
> > > -----Original Message-----
> > > From: Alex Williamson [mailto:alex.williamson@redhat.com]
> > > Sent: Wednesday, October 14, 2020 10:32 AM
> > > To: tiantao (H) <tiantao6@hisilicon.com>
> > > Cc: eric.auger@redhat.com; cohuck@redhat.com; kvm@vger.kernel.org;
> > > linux-kernel@vger.kernel.org; Song Bao Hua (Barry Song)
> > > <song.bao.hua@hisilicon.com>; Linuxarm <linuxarm@huawei.com>
> > > Subject: Re: [PATCH] vfio/platform: Replace spin_lock_irqsave by spin_lock
> in
> > > hard IRQ
> > >
> > > On Tue, 13 Oct 2020 10:00:58 +0800
> > > Tian Tao <tiantao6@hisilicon.com> wrote:
> > >
> > > > It is redundant to do irqsave and irqrestore in hardIRQ context.
> > >
> > > But this function is also called from non-IRQ context. Thanks,
> >
> > It seems you mean
> > vfio_platform_set_irqs_ioctl() ->
> > vfio_platform_set_irq_trigger ->
> > handler() ?
>
> Yes.
>
> > so, will it be better to move the irqsave out of the
> vfio_automasked_irq_handler()
> > and put it to where the function is called in non-IRQ context?
> >
> > I mean:
> >
> > irqhandler()
> > {
> > spin_lock() //without irqsave
> > spin_unlock()
> > }
> >
> > Non-irq context which is calling this handler:
> > irqsave();
> > irqhandler();
> > irqrestore();
> >
> > Anyway, if it is called in IRQ context, it is redundant to do irqsave.
>
> What's the advantage? You're saying it's redundant, is it also wrong?
It is not wrong and it doesn't make any malfunction. It just takes a couple of
instruction cycles to do save/restore and irq-disable/enable of cpu, which
is useless in irq context.
So the advantage is that we are going to remove some redundant instruction
cycles. And if the irq handler is called very often, we speed up the system.
> If it's not wrong and only redundant, what's the tangible latency
> difference in maintaining a separate IRQ context handler without the
> irqsave/restore? Thanks,
For this question, maybe need some benchmark to get answer. If the irqhandler
is not called that often, I agree it might be not worth to maintain two pieces of
code.
>
> Alex
>
> > > > Signed-off-by: Tian Tao <tiantao6@hisilicon.com>
> > > > ---
> > > > drivers/vfio/platform/vfio_platform_irq.c | 5 ++---
> > > > 1 file changed, 2 insertions(+), 3 deletions(-)
> > > >
> > > > diff --git a/drivers/vfio/platform/vfio_platform_irq.c
> > > b/drivers/vfio/platform/vfio_platform_irq.c
> > > > index c5b09ec..24fd6c5 100644
> > > > --- a/drivers/vfio/platform/vfio_platform_irq.c
> > > > +++ b/drivers/vfio/platform/vfio_platform_irq.c
> > > > @@ -139,10 +139,9 @@ static int vfio_platform_set_irq_unmask(struct
> > > vfio_platform_device *vdev,
> > > > static irqreturn_t vfio_automasked_irq_handler(int irq, void *dev_id)
> > > > {
> > > > struct vfio_platform_irq *irq_ctx = dev_id;
> > > > - unsigned long flags;
> > > > int ret = IRQ_NONE;
> > > >
> > > > - spin_lock_irqsave(&irq_ctx->lock, flags);
> > > > + spin_lock(&irq_ctx->lock);
> > > >
> > > > if (!irq_ctx->masked) {
> > > > ret = IRQ_HANDLED;
> > > > @@ -152,7 +151,7 @@ static irqreturn_t
> vfio_automasked_irq_handler(int
> > > irq, void *dev_id)
> > > > irq_ctx->masked = true;
> > > > }
> > > >
> > > > - spin_unlock_irqrestore(&irq_ctx->lock, flags);
> > > > + spin_unlock(&irq_ctx->lock);
> > > >
> > > > if (ret == IRQ_HANDLED)
> > > > eventfd_signal(irq_ctx->trigger, 1);
> >
Thanks
Barry
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2020-10-14 1:19 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-10-13 2:00 [PATCH] vfio/platform: Replace spin_lock_irqsave by spin_lock in hard IRQ Tian Tao
2020-10-13 21:32 ` Alex Williamson
2020-10-14 0:15 ` Song Bao Hua (Barry Song)
2020-10-14 0:50 ` Alex Williamson
2020-10-14 1:19 ` Song Bao Hua (Barry Song)
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).