Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1536241 > unrolled thread

Re: [PATCH] pci-hyperv: use kmalloc to allocate hypercall params buffer

Started byStephen Hemminger <stephen@networkplumber.org>
First post2016-12-05 18:00 +0100
Last post2016-12-06 17:30 +0100
Articles 3 — 2 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH] pci-hyperv: use kmalloc to allocate hypercall params  buffer Stephen Hemminger <stephen@networkplumber.org> - 2016-12-05 18:00 +0100
    RE: [PATCH] pci-hyperv: use kmalloc to allocate hypercall params  buffer Long Li <longli@microsoft.com> - 2016-12-06 04:10 +0100
      Re: [PATCH] pci-hyperv: use kmalloc to allocate hypercall params  buffer Stephen Hemminger <stephen@networkplumber.org> - 2016-12-06 17:30 +0100

#1536241 — Re: [PATCH] pci-hyperv: use kmalloc to allocate hypercall params buffer

FromStephen Hemminger <stephen@networkplumber.org>
Date2016-12-05 18:00 +0100
SubjectRe: [PATCH] pci-hyperv: use kmalloc to allocate hypercall params buffer
Message-ID<sL4Mq-71m-29@gated-at.bofh.it>
On Tue,  8 Nov 2016 14:04:38 -0800
Long Li <longli@exchange.microsoft.com> wrote:

> +	spin_lock_irqsave(&hbus->retarget_msi_interrupt_lock, flags);
> +
> +	params = &hbus->retarget_msi_interrupt_params;
> +	memset(params, 0, sizeof(*params));
> +	params->partition_id = HV_PARTITION_ID_SELF;
> +	params->source = 1; /* MSI(-X) */
> +	params->address = msi_desc->msg.address_lo;
> +	params->data = msi_desc->msg.data;
> +	params->device_id = (hbus->hdev->dev_instance.b[5] << 24) |
>  			   (hbus->hdev->dev_instance.b[4] << 16) |
>  			   (hbus->hdev->dev_instance.b[7] << 8) |
>  			   (hbus->hdev->dev_instance.b[6] & 0xf8) |
>  			   PCI_FUNC(pdev->devfn);
> -	params.vector = cfg->vector;
> +	params->vector = cfg->vector;
>  
>  	for_each_cpu_and(cpu, dest, cpu_online_mask)
> -		params.vp_mask |= (1ULL << vmbus_cpu_number_to_vp_number(cpu));
> +		params->vp_mask |= (1ULL << vmbus_cpu_number_to_vp_number(cpu));
> +
> +	hv_do_hypercall(HVCALL_RETARGET_INTERRUPT, params, NULL);
>  
> -	hv_do_hypercall(HVCALL_RETARGET_INTERRUPT, &params, NULL);
> +	spin_unlock_irqrestore(&hbus->retarget_msi_interrupt_lock, flags);

It looks like the additional locking here is being overly paranoid.
The caller is already holding the irq descriptor lock. Look at fixup_irqs.

[toc] | [next] | [standalone]

#1536624

FromLong Li <longli@microsoft.com>
Date2016-12-06 04:10 +0100
Message-ID<sLeiJ-4Me-1@gated-at.bofh.it>
In reply to#1536241

> -----Original Message-----
> From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> Sent: Monday, December 5, 2016 8:53 AM
> To: Long Li <longli@microsoft.com>
> Cc: KY Srinivasan <kys@microsoft.com>; Haiyang Zhang
> <haiyangz@microsoft.com>; Bjorn Helgaas <bhelgaas@google.com>;
> devel@linuxdriverproject.org; linux-kernel@vger.kernel.org; linux-
> pci@vger.kernel.org
> Subject: Re: [PATCH] pci-hyperv: use kmalloc to allocate hypercall params
> buffer
> 
> On Tue,  8 Nov 2016 14:04:38 -0800
> Long Li <longli@exchange.microsoft.com> wrote:
> 
> > +	spin_lock_irqsave(&hbus->retarget_msi_interrupt_lock, flags);
> > +
> > +	params = &hbus->retarget_msi_interrupt_params;
> > +	memset(params, 0, sizeof(*params));
> > +	params->partition_id = HV_PARTITION_ID_SELF;
> > +	params->source = 1; /* MSI(-X) */
> > +	params->address = msi_desc->msg.address_lo;
> > +	params->data = msi_desc->msg.data;
> > +	params->device_id = (hbus->hdev->dev_instance.b[5] << 24) |
> >  			   (hbus->hdev->dev_instance.b[4] << 16) |
> >  			   (hbus->hdev->dev_instance.b[7] << 8) |
> >  			   (hbus->hdev->dev_instance.b[6] & 0xf8) |
> >  			   PCI_FUNC(pdev->devfn);
> > -	params.vector = cfg->vector;
> > +	params->vector = cfg->vector;
> >
> >  	for_each_cpu_and(cpu, dest, cpu_online_mask)
> > -		params.vp_mask |= (1ULL <<
> vmbus_cpu_number_to_vp_number(cpu));
> > +		params->vp_mask |= (1ULL <<
> vmbus_cpu_number_to_vp_number(cpu));
> > +
> > +	hv_do_hypercall(HVCALL_RETARGET_INTERRUPT, params, NULL);
> >
> > -	hv_do_hypercall(HVCALL_RETARGET_INTERRUPT, &params, NULL);
> > +	spin_unlock_irqrestore(&hbus->retarget_msi_interrupt_lock, flags);
> 
> It looks like the additional locking here is being overly paranoid.
> The caller is already holding the irq descriptor lock. Look at fixup_irqs.

You are right. On my test machine, there are two possible places calling hv_irq_unmask(): request _irq() and handle_edge_irq(). They both have desc->lock held when calling .irq_unmask on the chip. A review of the IRQ code shows that desc->lock is always held while calling chip->irq_unmask().

Since the lock doesn't do any harm and it is not on performance code path, we can remove the lock in the upcoming patches.

[toc] | [prev] | [next] | [standalone]

#1537087

FromStephen Hemminger <stephen@networkplumber.org>
Date2016-12-06 17:30 +0100
Message-ID<sLqMV-4qH-7@gated-at.bofh.it>
In reply to#1536624
On Tue, 6 Dec 2016 00:37:08 +0000
Long Li <longli@microsoft.com> wrote:

> > -----Original Message-----
> > From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> > Sent: Monday, December 5, 2016 8:53 AM
> > To: Long Li <longli@microsoft.com>
> > Cc: KY Srinivasan <kys@microsoft.com>; Haiyang Zhang
> > <haiyangz@microsoft.com>; Bjorn Helgaas <bhelgaas@google.com>;
> > devel@linuxdriverproject.org; linux-kernel@vger.kernel.org; linux-
> > pci@vger.kernel.org
> > Subject: Re: [PATCH] pci-hyperv: use kmalloc to allocate hypercall params
> > buffer
> > 
> > On Tue,  8 Nov 2016 14:04:38 -0800
> > Long Li <longli@exchange.microsoft.com> wrote:
> >   
> > > +	spin_lock_irqsave(&hbus->retarget_msi_interrupt_lock, flags);
> > > +
> > > +	params = &hbus->retarget_msi_interrupt_params;
> > > +	memset(params, 0, sizeof(*params));
> > > +	params->partition_id = HV_PARTITION_ID_SELF;
> > > +	params->source = 1; /* MSI(-X) */
> > > +	params->address = msi_desc->msg.address_lo;
> > > +	params->data = msi_desc->msg.data;
> > > +	params->device_id = (hbus->hdev->dev_instance.b[5] << 24) |
> > >  			   (hbus->hdev->dev_instance.b[4] << 16) |
> > >  			   (hbus->hdev->dev_instance.b[7] << 8) |
> > >  			   (hbus->hdev->dev_instance.b[6] & 0xf8) |
> > >  			   PCI_FUNC(pdev->devfn);
> > > -	params.vector = cfg->vector;
> > > +	params->vector = cfg->vector;
> > >
> > >  	for_each_cpu_and(cpu, dest, cpu_online_mask)
> > > -		params.vp_mask |= (1ULL <<  
> > vmbus_cpu_number_to_vp_number(cpu));  
> > > +		params->vp_mask |= (1ULL <<  
> > vmbus_cpu_number_to_vp_number(cpu));  
> > > +
> > > +	hv_do_hypercall(HVCALL_RETARGET_INTERRUPT, params, NULL);
> > >
> > > -	hv_do_hypercall(HVCALL_RETARGET_INTERRUPT, &params, NULL);
> > > +	spin_unlock_irqrestore(&hbus->retarget_msi_interrupt_lock, flags);  
> > 
> > It looks like the additional locking here is being overly paranoid.
> > The caller is already holding the irq descriptor lock. Look at fixup_irqs.  
> 
> You are right. On my test machine, there are two possible places calling hv_irq_unmask(): request _irq() and handle_edge_irq(). They both have desc->lock held when calling .irq_unmask on the chip. A review of the IRQ code shows that desc->lock is always held while calling chip->irq_unmask().
> 
> Since the lock doesn't do any harm and it is not on performance code path, we can remove the lock in the upcoming patches.

Why add it then remove it, your patch hasn't been accepted. Please revise it and remove it.
Don't add additional unnecessary code.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web