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

Groups > linux.kernel > #1694109 > unrolled thread

Re: [PATCH v7 2/3] PCI: Enable PCIe Relaxed Ordering if supported

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 v7 2/3] PCI: Enable PCIe Relaxed Ordering if supported Ding Tianhong <dingtianhong@huawei.com> - 2017-07-22 06:30 +0200
    Re: [PATCH v7 2/3] PCI: Enable PCIe Relaxed Ordering if supported Alex Williamson <alex.williamson@redhat.com> - 2017-07-24 17:10 +0200
      Re: [PATCH v7 2/3] PCI: Enable PCIe Relaxed Ordering if supported Ding Tianhong <dingtianhong@huawei.com> - 2017-07-27 03:10 +0200
        Re: [PATCH v7 2/3] PCI: Enable PCIe Relaxed Ordering if supported "Raj, Ashok" <ashok.raj@intel.com> - 2017-07-27 20:50 +0200
          Re: [PATCH v7 2/3] PCI: Enable PCIe Relaxed Ordering if supported Ding Tianhong <dingtianhong@huawei.com> - 2017-07-28 05:00 +0200
        Re: [PATCH v7 2/3] PCI: Enable PCIe Relaxed Ordering if supported Ding Tianhong <dingtianhong@huawei.com> - 2017-07-28 05:00 +0200
      Re: [PATCH v7 2/3] PCI: Enable PCIe Relaxed Ordering if supported Ding Tianhong <dingtianhong@huawei.com> - 2017-07-27 03:20 +0200
        Re: [PATCH v7 2/3] PCI: Enable PCIe Relaxed Ordering if supported Alexander Duyck <alexander.duyck@gmail.com> - 2017-07-27 19:50 +0200
          Re: [PATCH v7 2/3] PCI: Enable PCIe Relaxed Ordering if supported Ding Tianhong <dingtianhong@huawei.com> - 2017-07-28 05:10 +0200

#1694109 — Re: [PATCH v7 2/3] PCI: Enable PCIe Relaxed Ordering if supported

Hi Sinan, Bjorn:

On 2017/7/14 21:54, Sinan Kaya wrote:
> On 7/13/2017 9:26 PM, Ding Tianhong wrote:
>> There is no code to enable the PCIe Relaxed Ordering bit in the configuration space,
>> it is only be enable by default according to the PCIe Standard Specification, what we
>> do is to distinguish the RC problematic platform and clear the Relaxed Ordering bit
>> to tell the PCIe EP don't send any TLPs with Relaxed Ordering Attributes to the Root
>> Complex.
> 
> Maybe, you should change the patch commit as 
> "Disable PCIe Relaxed Ordering if not supported"...

I agree that to use the new commit title as your suggested, thanks. :)

@Bjorn do you want me to spawn a new patchset with the new commit title
and the Reviewed-by from Casey on the patch 3, or maybe you could pick this
up and modify it own ? thanks.

Ding

> 

[toc] | [next] | [standalone]

#1694797

On Sat, 22 Jul 2017 12:19:38 +0800
Ding Tianhong <dingtianhong@huawei.com> wrote:

> Hi Sinan, Bjorn:
> 
> On 2017/7/14 21:54, Sinan Kaya wrote:
> > On 7/13/2017 9:26 PM, Ding Tianhong wrote:  
> >> There is no code to enable the PCIe Relaxed Ordering bit in the configuration space,
> >> it is only be enable by default according to the PCIe Standard Specification, what we
> >> do is to distinguish the RC problematic platform and clear the Relaxed Ordering bit
> >> to tell the PCIe EP don't send any TLPs with Relaxed Ordering Attributes to the Root
> >> Complex.  
> > 
> > Maybe, you should change the patch commit as 
> > "Disable PCIe Relaxed Ordering if not supported"...  
> 
> I agree that to use the new commit title as your suggested, thanks. :)
> 
> @Bjorn do you want me to spawn a new patchset with the new commit title
> and the Reviewed-by from Casey on the patch 3, or maybe you could pick this
> up and modify it own ? thanks.

Hi Ding,

Bjorn is currently on holiday so it might be a good idea to respin the
series with any updates so nothing is lost.  Thanks,

Alex

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

#1697646

On 2017/7/27 3:05, Casey Leedom wrote:
> | From: Alexander Duyck <alexander.duyck@gmail.com>
> | Sent: Wednesday, July 26, 2017 11:44 AM
> | 
> | On Jul 26, 2017 11:26 AM, "Casey Leedom" <leedom@chelsio.com> wrote:
> | |
> | |     I think that the patch will need to be extended to modify
> | |     drivers/pci.c/iov.c:sriov_enable() to explicitly turn off
> | |     Relaxed Ordering Enable if the Root Complex is marked
> |     for no RO TLPs.
> | 
> | I'm not sure that would be an issue. Wouldn't most VFs inherit the PF's settings?
> 
> Ah yes, you're right.  This is covered in section 3.5.4 of the Single Root I/O
> Virtualization and Sharing Specification, Revision 1.0 (September 11, 2007),
> governing the PCIe Capability Device Control register.  It states that the VF
> version of that register shall follow the setting of the corresponding PF.
> 
> So we should enhance the cxgb4vf/sge.c:t4vf_sge_alloc_rxq() in the same
> way we did for the cxgb4 driver, but that's not critical since the Relaxed
> Ordering Enable supersedes the internal chip's desire to use the Relaxed
> Ordering Attribute.
> 
> Ding, send me a note if you'd like me to work that up for you.
> 

Ok, you could send the change log and I could put it in the v8 version together,
will you base on the patch 3/3 or build a independence patch?

Ding

> | Also I thought most of the VF configuration space is read only.
> 
> Yes, but not all of it.  And when a VF is exported to a Virtual Machine,
> then the Hypervisor captures and interprets all accesses to the VF's
> PCIe Configuration Space from the VM.
> 
> Thanks again for reminding me of the subtle aspect of the SR_IOV
> specification that I forgot.
> 
> Casey
> .
> 

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

#1698248

Hi Casey

> | Still no Intel and AMD guys has ack this, this is what I am worried about,
> | should I ping some man again ?


I can ack the patch set for Intel specific changes. Now that the doc is made
public :-).

Can you/Ding resend the patch series, i do have the most recent v7, some
of the commit message wasn't easy to ready. Seems like this patch has
gotten bigger than originally intended, but seems to be for the overall
good :-).

Sorry for staying silent up until now.

Cheers,
Ashok

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

#1698430

On 2017/7/28 2:42, Raj, Ashok wrote:
> Hi Casey
> 
>> | Still no Intel and AMD guys has ack this, this is what I am worried about,
>> | should I ping some man again ?
> 
> 
> I can ack the patch set for Intel specific changes. Now that the doc is made
> public :-).
> 

Good, Thanks. :)

> Can you/Ding resend the patch series, i do have the most recent v7, some
> of the commit message wasn't easy to ready. Seems like this patch has
> gotten bigger than originally intended, but seems to be for the overall
> good :-).
> 

OK, I will send v8 patch set and which will update the patch title and add
Casey's new modification for his vf driver, thanks.

Ding

> Sorry for staying silent up until now.
> 
> Cheers,
> Ashok
> 
> .
> 

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

#1698431

On 2017/7/28 1:44, Casey Leedom wrote:
> | From: Ding Tianhong <dingtianhong@huawei.com>
> | Sent: Wednesday, July 26, 2017 6:01 PM
> |
> | On 2017/7/27 3:05, Casey Leedom wrote:
> | >
> | > Ding, send me a note if you'd like me to work that [cxgb4vf patch] up
> | > for you.
> |
> | Ok, you could send the change log and I could put it in the v8 version
> | together, will you base on the patch 3/3 or build a independence patch?
> 
> Which ever you'd prefer.  It would basically mirror the same exact code that
> you've got for cxgb4.  I.e. testing the setting of the VF's PCIe Capability
> Device Control[Relaxed Ordering Enable], setting a new flag in
> adpater->flags, testing that flag in cxgb4vf/sge.c:t4vf_sge_alloc_rxq().
> But since the VF's PF will already have disabled the PF's Relaxed Ordering
> Enable, the VF will also have it's Relaxed Ordering Enable disabled and any
> effort by the internal chip to send TLPs with the Relaxed Ordering Attribute
> will be gated by the PCIe logic.  So it's not critical that this be in the
> first patch.  Your call.  Let me know if you'd like me to send that to you.
> 

Good， please Send it to me, I will put it together and send the v8 this week,
I think Bjorn will be back next week .:)

> 
> | From: Ding Tianhong <dingtianhong@huawei.com>
> | Sent: Wednesday, July 26, 2017 6:08 PM
> |
> | On 2017/7/27 2:26, Casey Leedom wrote:
> | >
> | >  1. Did we ever get any acknowledgement from either Intel or AMD
> | >     on this patch?  I know that we can't ensure that, but it sure would
> | >     be nice since the PCI Quirks that we're putting in affect their
> | >     products.
> |
> | Still no Intel and AMD guys has ack this, this is what I am worried about,
> | should I ping some man again ?
> 
> By amusing coincidence, Patrik Cramer (now Cc'ed) from Intel sent me a note
> yesterday with a link to the official Intel performance tuning documentation
> which covers this issue:
> 
> https://software.intel.com/sites/default/files/managed/9e/bc/64-ia-32-architectures-optimization-manual.pdf
> 
> In section 3.9.1 we have:
> 
>     3.9.1 Optimizing PCIe Performance for Accesses Toward Coherent Memory
>           and Toward MMIO Regions (P2P)
> 
>     In order to maximize performance for PCIe devices in the processors
>     listed in Table 3-6 below, the soft- ware should determine whether the
>     accesses are toward coherent memory (system memory) or toward MMIO
>     regions (P2P access to other devices). If the access is toward MMIO
>     region, then software can command HW to set the RO bit in the TLP
>     header, as this would allow hardware to achieve maximum throughput for
>     these types of accesses. For accesses toward coherent memory, software
>     can command HW to clear the RO bit in the TLP header (no RO), as this
>     would allow hardware to achieve maximum throughput for these types of
>     accesses.
> 
>     Table 3-6. Intel Processor CPU RP Device IDs for Processors Optimizing
>                PCIe Performance
> 
>     Processor                            CPU RP Device IDs
> 
>     Intel Xeon processors based on       6F01H-6F0EH
>     Broadwell microarchitecture
> 
>     Intel Xeon processors based on       2F01H-2F0EH
>     Haswell microarchitecture
> 
> Unfortunately that's a pretty thin section.  But it does expand the set of
> Intel Root Complexes for which our Linux PCI Quirk will need to cover.  So
> you should add those to the next (and hopefully final) spin of your patch.
> And, it also verifies the need to handle the use of Relaxed Ordering more
> subtlely than simply turning it off since the NVMe peer-to-peer example I
> keep bringing up would fall into the "need to use Relaxed Ordering" case ...
> 
> It would have been nice to know why this is happening and if any future
> processor would fix this.  After all, Relaxed Ordering, is just supposed to
> be a hint.  At worst, a receiving device could just ignore the attribute
> entirely.  Obviously someone made an effort to implement it but ... it
> didn't go the way they wanted.
> 
> And, it also would have been nice to know if there was any hidden register
> in these Intel Root Complexes which can completely turn off the effort to
> pay attention to the Relaxed Ordering Attribute.  We've spend an enormous
> amount of effort on this issue here on the Linux PCI email list struggling
> mightily to come up with a way to determine when it's
> safe/recommended/not-recommended/unsafe to use Relaxed Ordering when
> directing TLPs towards the Root Complex.  And some architectures require RO
> for decent performance so we can't just "turn it off" unilatterally.
> 

I am glad to hear that more person were focus on this problem, It would be great
if they could enter our discussion and give us more suggestion. :)

Thanks
Ding

> Casey
> 
> .
> 

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

#1697647

On 2017/7/27 2:26, Casey Leedom wrote:
>   By the way Ding, two issues:
> 
>  1. Did we ever get any acknowledgement from either Intel or AMD
>     on this patch?  I know that we can't ensure that, but it sure would
>     be nice since the PCI Quirks that we're putting in affect their
>     products.
> 

Still no Intel and AMD guys has ack this, this is what I am worried about, should I
ping some man again ?

Thanks
Ding
> 
> Casey
> .
> 

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

#1698218

On Wed, Jul 26, 2017 at 6:08 PM, Ding Tianhong <dingtianhong@huawei.com> wrote:
>
>
> On 2017/7/27 2:26, Casey Leedom wrote:
>>   By the way Ding, two issues:
>>
>>  1. Did we ever get any acknowledgement from either Intel or AMD
>>     on this patch?  I know that we can't ensure that, but it sure would
>>     be nice since the PCI Quirks that we're putting in affect their
>>     products.
>>
>
> Still no Intel and AMD guys has ack this, this is what I am worried about, should I
> ping some man again ?
>
> Thanks
> Ding


I probably wouldn't worry about it too much. If anything all this
patch is doing is disabling relaxed ordering on the platforms we know
have issues based on what Casey originally had. If nothing else we can
follow up once the patches are in the kernel and if somebody has an
issue then.

You can include my acked-by, but it is mostly related to how this
interacts with NICs, and not so much about the PCI chipsets
themselves.

Acked-by: Alexander Duyck <alexander.h.duyck@intel.com>

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

#1698434

On 2017/7/28 1:49, Alexander Duyck wrote:
> On Wed, Jul 26, 2017 at 6:08 PM, Ding Tianhong <dingtianhong@huawei.com> wrote:
>>
>>
>> On 2017/7/27 2:26, Casey Leedom wrote:
>>>   By the way Ding, two issues:
>>>
>>>  1. Did we ever get any acknowledgement from either Intel or AMD
>>>     on this patch?  I know that we can't ensure that, but it sure would
>>>     be nice since the PCI Quirks that we're putting in affect their
>>>     products.
>>>
>>
>> Still no Intel and AMD guys has ack this, this is what I am worried about, should I
>> ping some man again ?
>>
>> Thanks
>> Ding
> 
> 
> I probably wouldn't worry about it too much. If anything all this
> patch is doing is disabling relaxed ordering on the platforms we know
> have issues based on what Casey originally had. If nothing else we can
> follow up once the patches are in the kernel and if somebody has an
> issue then.
> 
> You can include my acked-by, but it is mostly related to how this
> interacts with NICs, and not so much about the PCI chipsets
> themselves.
> 
> Acked-by: Alexander Duyck <alexander.h.duyck@intel.com>
> 

Thanks, Alex. :)

> .
> 

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web