[PATCH] PCI: pciehp: Use ordered workqueue for HPC events

[PATCH] PCI: pciehp: Use ordered workqueue for HPC events

[Sandeep Mann]

Using an ordered workqueue serializes processing of hotplug event. Processing
an hotplug event for some devices can take a relatively "long" time, which
means if a device is added and removed in quick succession (or due to flacky
hardware), it can lead to multiple outstanding hotplug events. Processing
these events concurrently can lead to unknown internal state and/or kernel
panics.

Signed-off-by: Sandeep Mann <sandeep@purestorage.com>
---
 drivers/pci/hotplug/pciehp_hpc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
index 42914e0..c4eedab 100644
--- a/drivers/pci/hotplug/pciehp_hpc.c
+++ b/drivers/pci/hotplug/pciehp_hpc.c
@@ -681,7 +681,7 @@ static int pcie_init_slot(struct controller *ctrl)
 	if (!slot)
 		return -ENOMEM;
 
-	slot->wq = alloc_workqueue("pciehp-%u", 0, 0, PSN(ctrl));
+	slot->wq = alloc_ordered_workqueue("pciehp-%u", 0, PSN(ctrl));
 	if (!slot->wq)
 		goto abort;
 
-- 
1.8.3.2

RE: [PATCH] PCI: pciehp: Use ordered workqueue for HPC events

[Rajat Jain]

> -----Original Message-----
> From: linux-pci-owner@vger.kernel.org [mailto:linux-pci-
> owner@vger.kernel.org] On Behalf Of Sandeep Mann
> Sent: Friday, August 01, 2014 3:47 PM
> To: bhelgaas@google.com; linux-pci@vger.kernel.org
> Subject: [PATCH] PCI: pciehp: Use ordered workqueue for HPC events
> 
> Using an ordered workqueue serializes processing of hotplug event.
> Processing an hotplug event for some devices can take a relatively "long"
> time, which means if a device is added and removed in quick succession (or
> due to flacky hardware), it can lead to multiple outstanding hotplug events.
> Processing these events concurrently can lead to unknown internal state
> and/or kernel panics.
> 
> Signed-off-by: Sandeep Mann <sandeep@purestorage.com>

Acked-by: Rajat Jain <rajatxjain@gmail.com>

Yes, this is good and needed. Sometime back I felt the need for it, but sort of assumed that the slot->wq is already ordered.

Thanks,
Rajat


> ---
>  drivers/pci/hotplug/pciehp_hpc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/hotplug/pciehp_hpc.c
> b/drivers/pci/hotplug/pciehp_hpc.c
> index 42914e0..c4eedab 100644
> --- a/drivers/pci/hotplug/pciehp_hpc.c
> +++ b/drivers/pci/hotplug/pciehp_hpc.c
> @@ -681,7 +681,7 @@ static int pcie_init_slot(struct controller *ctrl)
>  	if (!slot)
>  		return -ENOMEM;
> 
> -	slot->wq = alloc_workqueue("pciehp-%u", 0, 0, PSN(ctrl));
> +	slot->wq = alloc_ordered_workqueue("pciehp-%u", 0, PSN(ctrl));
>  	if (!slot->wq)
>  		goto abort;
> 
> --
> 1.8.3.2
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in the
> body of a message to majordomo@vger.kernel.org More majordomo info at
> http://vger.kernel.org/majordomo-info.html

Re: [PATCH] PCI: pciehp: Use ordered workqueue for HPC events

[Bjorn Helgaas]

[+cc Rajat]

On Fri, Aug 01, 2014 at 03:47:12PM -0700, Sandeep Mann wrote:
> Using an ordered workqueue serializes processing of hotplug event. Processing
> an hotplug event for some devices can take a relatively "long" time, which
> means if a device is added and removed in quick succession (or due to flacky
> hardware), it can lead to multiple outstanding hotplug events. Processing
> these events concurrently can lead to unknown internal state and/or kernel
> panics.

Did you find this by code inspection, or did you actually observe a
problem?  If the latter, what were the symptoms?

shpchp does the same thing, so I assume it requires the same fix.

> Signed-off-by: Sandeep Mann <sandeep@purestorage.com>
> ---
>  drivers/pci/hotplug/pciehp_hpc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
> index 42914e0..c4eedab 100644
> --- a/drivers/pci/hotplug/pciehp_hpc.c
> +++ b/drivers/pci/hotplug/pciehp_hpc.c
> @@ -681,7 +681,7 @@ static int pcie_init_slot(struct controller *ctrl)
>  	if (!slot)
>  		return -ENOMEM;
>  
> -	slot->wq = alloc_workqueue("pciehp-%u", 0, 0, PSN(ctrl));
> +	slot->wq = alloc_ordered_workqueue("pciehp-%u", 0, PSN(ctrl));
>  	if (!slot->wq)
>  		goto abort;
>  
> -- 
> 1.8.3.2
> 

Re: [PATCH] PCI: pciehp: Use ordered workqueue for HPC events

[Sandeep S Mann]

> Did you find this by code inspection, or did you actually observe a
> problem?  If the latter, what were the symptoms?
> 
> shpchp does the same thing, so I assume it requires the same fix.

Hi Bjorn,

This issue was observed on a system, where I was experimenting with
pcie hot-plug and the device being hot plugged is not the most well
behaved.  It exhibits the following behavior:

  - Initialization of the device can take “relatively” long (order 100s ms)
  - On hot add the link presence can toggle before stabilizing

The toggle of device presence can generate multiple events on a single
hot plug, where one hot add event can lead to following events
ADD->REMOVE->ADD.  And given the device can take some time to initialize,
we can have three HP events being processed concurrently.

Also note this seen with surprise ADD and REMOVE.

S

Re: [PATCH] PCI: pciehp: Use ordered workqueue for HPC events

[Rajat Jain]

Hi,

On Tue, Aug 12, 2014 at 3:30 PM, Sandeep S Mann <sandeep@purestorage.com> wrote:
>
>> Did you find this by code inspection, or did you actually observe a
>> problem?  If the latter, what were the symptoms?
>>
>> shpchp does the same thing, so I assume it requires the same fix.
>
> Hi Bjorn,
>
> This issue was observed on a system, where I was experimenting with
> pcie hot-plug and the device being hot plugged is not the most well
> behaved.  It exhibits the following behavior:
>
>   - Initialization of the device can take “relatively” long (order 100s ms)
>   - On hot add the link presence can toggle before stabilizing

Yup, this would be a problem if the work queue was reordered in this case.

>
> The toggle of device presence can generate multiple events on a single
> hot plug, where one hot add event can lead to following events
> ADD->REMOVE->ADD.  And given the device can take some time to initialize,
> we can have three HP events being processed concurrently.

The HP events are actually serialized, but only from the perspective
if PCI subsystem (i.e. once they have been dispatched from the
workqueue):
https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=50b52fdee050745935d92e7026373edea2647e60

>
> Also note this seen with surprise ADD and REMOVE.
>

Can you please send lspci -vvvv for the slot? I suspect that your
device has a power controller. In absense of it, the link state
changes would generate HP events regardless of "surprise" bit.

Thanks,

Rajat

> S
>
>

Re: [PATCH] PCI: pciehp: Use ordered workqueue for HPC events

[Sandeep S Mann]

Hi Rajat,


>> 
>> This issue was observed on a system, where I was experimenting with
>> pcie hot-plug and the device being hot plugged is not the most well
>> behaved.  It exhibits the following behavior:
>> 
>>  - Initialization of the device can take “relatively” long (order 100s ms)
>>  - On hot add the link presence can toggle before stabilizing
> 
> Yup, this would be a problem if the work queue was reordered in this case.
> 
>> 
>> The toggle of device presence can generate multiple events on a single
>> hot plug, where one hot add event can lead to following events
>> ADD->REMOVE->ADD.  And given the device can take some time to initialize,
>> we can have three HP events being processed concurrently.
> 
> The HP events are actually serialized, but only from the perspective
> if PCI subsystem (i.e. once they have been dispatched from the
> workqueue):
> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=50b52fdee050745935d92e7026373edea2647e60
> 

With an ordered workqueue I am wondering if the locking is needed.  My kernel tree
seems to be a bit behind and does not contain this change.  I suppose there
could still be a race between pciehp_probe and the processing of the HP event.  

>> 
>> Also note this seen with surprise ADD and REMOVE.
>> 
> 
> Can you please send lspci -vvvv for the slot? I suspect that your
> device has a power controller. In absense of it, the link state
> changes would generate HP events regardless of "surprise" bit.

Yes we are behind a slot with power management.

        Capabilities: [40] Power Management version 3
                Flags: PMEClk- DSI- D1- D2- AuxCurrent=0mA PME(D0+,D1-,D2-,D3hot+,D3cold+)
                Status: D0 NoSoftRst+ PME-Enable- DSel=0 DScale=0 PME-
Thanks,
Sandeep

Re: [PATCH] PCI: pciehp: Use ordered workqueue for HPC events

[Rajat Jain]

Hi,

>> Can you please send lspci -vvvv for the slot? I suspect that your
>> device has a power controller. In absense of it, the link state
>> changes would generate HP events regardless of "surprise" bit.
>
> Yes we are behind a slot with power management.
>
>         Capabilities: [40] Power Management version 3
>                 Flags: PMEClk- DSI- D1- D2- AuxCurrent=0mA PME(D0+,D1-,D2-,D3hot+,D3cold+)
>                 Status: D0 NoSoftRst+ PME-Enable- DSel=0 DScale=0 PME-

Actually I was referring to the power controller for the hotplug slot
i.e. in the Slot capabilities register.

However, it is a moot point now since you mentioned that you tree is
behind the current one (these changes are present in 3.15 onwards).

Thanks,

Rajat

Re: [PATCH] PCI: pciehp: Use ordered workqueue for HPC events

[Sandeep S Mann]

On Aug 12, 2014, at 5:33 PM, Rajat Jain <rajatxjain@gmail.com> wrote:

> Actually I was referring to the power controller for the hotplug slot
> i.e. in the Slot capabilities register.

Here is the capability.

        Capabilities: [68] Express (v2) Downstream Port (Slot+), MSI 00
                DevCap: MaxPayload 512 bytes, PhantFunc 0, Latency L0s <64ns, L1 <1us
                        ExtTag- RBE+ FLReset-
                DevCtl: Report errors: Correctable+ Non-Fatal+ Fatal+ Unsupported+
                        RlxdOrd+ ExtTag- PhantFunc- AuxPwr- NoSnoop+
                        MaxPayload 128 bytes, MaxReadReq 128 bytes
                DevSta: CorrErr+ UncorrErr- FatalErr- UnsuppReq+ AuxPwr- TransPend-
                LnkCap: Port #4, Speed 8GT/s, Width x4, ASPM L1, Latency L0 <4us, L1 <4us
                        ClockPM- Surprise+ LLActRep+ BwNot+
                LnkCtl: ASPM Disabled; Disabled- Retrain- CommClk-
                        ExtSynch- ClockPM- AutWidDis- BWInt- AutBWInt-
                LnkSta: Speed 2.5GT/s, Width x0, TrErr- Train- SlotClk- DLActive- BWMgmt- ABWMgmt-
                SltCap: AttnBtn- PwrCtrl- MRL- AttnInd- PwrInd- HotPlug+ Surprise+
                        Slot #300, PowerLimit 25.000W; Interlock- NoCompl-
                SltCtl: Enable: AttnBtn- PwrFlt- MRL- PresDet+ CmdCplt- HPIrq+ LinkChg-
                        Control: AttnInd Unknown, PwrInd Unknown, Power- Interlock-
                SltSta: Status: AttnBtn- PowerFlt- MRL- CmdCplt- PresDet- Interlock-
                        Changed: MRL- PresDet- LinkState-
                DevCap2: Completion Timeout: Not Supported, TimeoutDis- ARIFwd+
                DevCtl2: Completion Timeout: 50us to 50ms, TimeoutDis- ARIFwd-
                LnkCtl2: Target Link Speed: 8GT/s, EnterCompliance- SpeedDis-, Selectable De-emphasis: -6dB
                         Transmit Margin: Normal Operating Range, EnterModifiedCompliance- ComplianceSOS-
                         Compliance De-emphasis: -6dB
                LnkSta2: Current De-emphasis Level: -3.5dB, EqualizationComplete-, EqualizationPhase1-
                         EqualizationPhase2-, EqualizationPhase3-, LinkEqualizationRequest-


> However, it is a moot point now since you mentioned that you tree is
> behind the current one (these changes are present in 3.15 onwards).

Thanks we will pick up these changes once we move forward to 3.15.

Re: [PATCH] PCI: pciehp: Use ordered workqueue for HPC events

[Bjorn Helgaas]

On Fri, Aug 01, 2014 at 03:47:12PM -0700, Sandeep Mann wrote:
> Using an ordered workqueue serializes processing of hotplug event. Processing
> an hotplug event for some devices can take a relatively "long" time, which
> means if a device is added and removed in quick succession (or due to flacky
> hardware), it can lead to multiple outstanding hotplug events. Processing
> these events concurrently can lead to unknown internal state and/or kernel
> panics.
> 
> Signed-off-by: Sandeep Mann <sandeep@purestorage.com>

What's the status of this?  I see Rajat's ack, but there was a lot of
subsequent discussion, and I'm not sure whether there was a conclusion.

Given that, I'd like to see a bugzilla with the relevant background
(lspci -vv, dmesg log).  Also, shpchp_core.c has similar code, so I'd like
to see either a similar patch for it or an explanation of why that driver
doesn't need it.

So I'll drop this for now, pending these updates.

Bjorn

> ---
>  drivers/pci/hotplug/pciehp_hpc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
> index 42914e0..c4eedab 100644
> --- a/drivers/pci/hotplug/pciehp_hpc.c
> +++ b/drivers/pci/hotplug/pciehp_hpc.c
> @@ -681,7 +681,7 @@ static int pcie_init_slot(struct controller *ctrl)
>  	if (!slot)
>  		return -ENOMEM;
>  
> -	slot->wq = alloc_workqueue("pciehp-%u", 0, 0, PSN(ctrl));
> +	slot->wq = alloc_ordered_workqueue("pciehp-%u", 0, PSN(ctrl));
>  	if (!slot->wq)
>  		goto abort;
>  
> -- 
> 1.8.3.2
> 

RE: [PATCH] PCI: pciehp: Use ordered workqueue for HPC events

[Rajat Jain]

Hello,

> -----Original Message-----
> From: linux-pci-owner@vger.kernel.org [mailto:linux-pci-
> owner@vger.kernel.org] On Behalf Of Bjorn Helgaas
> Sent: Thursday, September 04, 2014 2:11 PM
> To: Sandeep Mann
> Cc: linux-pci@vger.kernel.org
> Subject: Re: [PATCH] PCI: pciehp: Use ordered workqueue for HPC events
> 
> On Fri, Aug 01, 2014 at 03:47:12PM -0700, Sandeep Mann wrote:
> > Using an ordered workqueue serializes processing of hotplug event.
> > Processing an hotplug event for some devices can take a relatively
> > "long" time, which means if a device is added and removed in quick
> > succession (or due to flacky hardware), it can lead to multiple
> > outstanding hotplug events. Processing these events concurrently can
> > lead to unknown internal state and/or kernel panics.
> >
> > Signed-off-by: Sandeep Mann <sandeep@purestorage.com>
Acked-by: Rajat Jain <rajatxjain@gmail.com>

> 
> What's the status of this?  I see Rajat's ack, but there was a lot of subsequent
> discussion, and I'm not sure whether there was a conclusion.

Yes, we concluded that the patch was needed.

> 
> Given that, I'd like to see a bugzilla with the relevant background (lspci -vv,
> dmesg log). 

Sandeep, is that something you can provide?

> Also, shpchp_core.c has similar code, so I'd like to see either a
> similar patch for it or an explanation of why that driver doesn't need it.
> 

I can provide that patch. I took a look at the commit history of SHPCHP and PCIEHP
And I see a bunch of commits that switch to using the unordered work queues.

d347e75847 ("PCI: shpchp: Handle push button event asynchronously")
10959d72d4 ("PCI: shpchp: Make shpchp_wq non-ordered")
486b10b9f4 ("PCI: pciehp: Handle push button event asynchronously")
e24dcbef93 ("shpchp: update workqueue usage")
a827ea307b ("pciehp: update workqueue usage")

While I'm still trying to understand if there are scenarios where ordered queues can be a problem,
I'm able to find convincing scenarios where having an unordered queue will fail the driver.

Using ordered queues are definitely a requirement for cases where
hotplug events can happen very fast enough not to give SW the time to
handle them. (For e.g., for pciehp: PCIe link fluctuations may result in
such events).

Problem:
     Consider a very fast ocuurance of HOTPLUG->UNPLUG->HOTPLUG
     (fast enough that it does not give the wq to get a chance to run)

     If the wq can be reordered, it is a problem if the worker function
     is invoked as HOTPLUG->HOTPLUG->UNPLUG.

Why:
     In the above example, the final result should have been a device
     added, but if it is reordered like above, thre shall be no device
     added. The second HOTPLUG event shall be treated as a stray one
     since a device is already present there (""Cannot add device at..")

Thus we need to have ordered work queues. 

The primary reasons to switch to using unordered workqueues as far
as I could understand from the logs were (1) avoid using the system
workqueue, and (2) let the button events be handled asynchronously.

Do you think a ordered workqueue not work for any of the reasons above?

Thanks,

Rajat

(PS: In short, I think the ordered workqueues are definitely needed for pciehp, specially with the inclusion of link-state hotplug. I think it will also help the shpchp - but I'm still trying to understand if there are any uncovered scenarios).


> So I'll drop this for now, pending these updates.
> 
> Bjorn
> 
> > ---
> >  drivers/pci/hotplug/pciehp_hpc.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/pci/hotplug/pciehp_hpc.c
> > b/drivers/pci/hotplug/pciehp_hpc.c
> > index 42914e0..c4eedab 100644
> > --- a/drivers/pci/hotplug/pciehp_hpc.c
> > +++ b/drivers/pci/hotplug/pciehp_hpc.c
> > @@ -681,7 +681,7 @@ static int pcie_init_slot(struct controller *ctrl)
> >  	if (!slot)
> >  		return -ENOMEM;
> >
> > -	slot->wq = alloc_workqueue("pciehp-%u", 0, 0, PSN(ctrl));
> > +	slot->wq = alloc_ordered_workqueue("pciehp-%u", 0, PSN(ctrl));
> >  	if (!slot->wq)
> >  		goto abort;
> >
> > --
> > 1.8.3.2
> >
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in the
> body of a message to majordomo@vger.kernel.org More majordomo info at
> http://vger.kernel.org/majordomo-info.html