linux-kernel - Re: [PATCH] vfio/platform: Replace spin_lock_irqsave by spin

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20201013185023.455a6ca9@x1.home>
Date:   Tue, 13 Oct 2020 18:50:23 -0600
From:   Alex Williamson <alex.williamson@...hat.com>
To:     "Song Bao Hua (Barry Song)" <song.bao.hua@...ilicon.com>
Cc:     "tiantao (H)" <tiantao6@...ilicon.com>,
        "eric.auger@...hat.com" <eric.auger@...hat.com>,
        "cohuck@...hat.com" <cohuck@...hat.com>,
        "kvm@...r.kernel.org" <kvm@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Linuxarm <linuxarm@...wei.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@...ilicon.com> wrote:

> > -----Original Message-----
> > From: Alex Williamson [mailto:alex.williamson@...hat.com]
> > Sent: Wednesday, October 14, 2020 10:32 AM
> > To: tiantao (H) <tiantao6@...ilicon.com>
> > Cc: eric.auger@...hat.com; cohuck@...hat.com; kvm@...r.kernel.org;
> > linux-kernel@...r.kernel.org; Song Bao Hua (Barry Song)
> > <song.bao.hua@...ilicon.com>; Linuxarm <linuxarm@...wei.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@...ilicon.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@...ilicon.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
>