| Message ID | 4A3B03D5.7090200@jp.fujitsu.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
On Fri, Jun 19, 2009 at 11:19:49AM +0800, Kenji Kaneshige wrote: > Shaohua Li wrote: > > On Fri, Jun 19, 2009 at 05:28:41AM +0800, Jesse Barnes wrote: > >> On Thu, 21 May 2009 11:10:06 +0900 > >> Kenji Kaneshige <kaneshige.kenji@jp.fujitsu.com> wrote: > >> > >>> Hi, > >>> > >>> This patch changes ASPM driver to enable more detailed ASPM > >>> configuration. With this patch, > >>> > >>> - ASPM can be enabled partially in the hierarchy. > >>> - L0s can be managed for each direction (upstream direction and > >>> downstream direction) of the link. > > Well, these are nice features, but what kind of machine does you test? > > Does it have several level hierarchy? The most powerful system I saw > > just has two levels, which sounds not worth adding partial aspm. > > Thank you very much for comments. > > I think it is useful also for the system that doesn't have so > many levels of hierarchy, such as mobile systems. Here is "lspci -t" > output on my environment with some comments. My system also has > just two levels, but with the patch, upstream L0s is enabled on the > link 0000:15.00.0 (switch downstream port) to 0000:16:00.0 (endpoint) > in powersave mode. Can you send me the output of lspci -vvvxxx please? > (*) Downstream direction L0s is enabled but it has no effect because > Downstream direction L0s is disabled on its upstream links. > > > The second feature seems more reasonable. > > I think I should have implement those features as separate patches. > I'll do it if needed. yes, please. Thanks, Shaohua -- 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
On Fri, Jun 19, 2009 at 01:28:44PM +0800, Kenji Kaneshige wrote: > Shaohua Li wrote: > > On Fri, Jun 19, 2009 at 11:19:49AM +0800, Kenji Kaneshige wrote: > >> Shaohua Li wrote: > >>> On Fri, Jun 19, 2009 at 05:28:41AM +0800, Jesse Barnes wrote: > >>>> On Thu, 21 May 2009 11:10:06 +0900 > >>>> Kenji Kaneshige <kaneshige.kenji@jp.fujitsu.com> wrote: > >>>> > >>>>> Hi, > >>>>> > >>>>> This patch changes ASPM driver to enable more detailed ASPM > >>>>> configuration. With this patch, > >>>>> > >>>>> - ASPM can be enabled partially in the hierarchy. > >>>>> - L0s can be managed for each direction (upstream direction and > >>>>> downstream direction) of the link. > >>> Well, these are nice features, but what kind of machine does you test? > >>> Does it have several level hierarchy? The most powerful system I saw > >>> just has two levels, which sounds not worth adding partial aspm. > >> Thank you very much for comments. > >> > >> I think it is useful also for the system that doesn't have so > >> many levels of hierarchy, such as mobile systems. Here is "lspci -t" > >> output on my environment with some comments. My system also has > >> just two levels, but with the patch, upstream L0s is enabled on the > >> link 0000:15.00.0 (switch downstream port) to 0000:16:00.0 (endpoint) > >> in powersave mode. > > Can you send me the output of lspci -vvvxxx please? > > > > Here it is. Please note that I'm using pcie_aspm=force option. Hi, can you please split the patch into two patches, then I will have more detail review/test. Also please don't do cleanup in the patches for new feature. I had a simple look at the patch, and found at least one issue: A link might have multiple child links, we need check all child links's latency to enable the parent's ASPM. In pcie_config_aspm_path(), the patch seems not check peer link. Thanks, Shaohua -- 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
Shaohua Li wrote: > On Fri, Jun 19, 2009 at 01:28:44PM +0800, Kenji Kaneshige wrote: >> Shaohua Li wrote: >>> On Fri, Jun 19, 2009 at 11:19:49AM +0800, Kenji Kaneshige wrote: >>>> Shaohua Li wrote: >>>>> On Fri, Jun 19, 2009 at 05:28:41AM +0800, Jesse Barnes wrote: >>>>>> On Thu, 21 May 2009 11:10:06 +0900 >>>>>> Kenji Kaneshige <kaneshige.kenji@jp.fujitsu.com> wrote: >>>>>> >>>>>>> Hi, >>>>>>> >>>>>>> This patch changes ASPM driver to enable more detailed ASPM >>>>>>> configuration. With this patch, >>>>>>> >>>>>>> - ASPM can be enabled partially in the hierarchy. >>>>>>> - L0s can be managed for each direction (upstream direction and >>>>>>> downstream direction) of the link. >>>>> Well, these are nice features, but what kind of machine does you test? >>>>> Does it have several level hierarchy? The most powerful system I saw >>>>> just has two levels, which sounds not worth adding partial aspm. >>>> Thank you very much for comments. >>>> >>>> I think it is useful also for the system that doesn't have so >>>> many levels of hierarchy, such as mobile systems. Here is "lspci -t" >>>> output on my environment with some comments. My system also has >>>> just two levels, but with the patch, upstream L0s is enabled on the >>>> link 0000:15.00.0 (switch downstream port) to 0000:16:00.0 (endpoint) >>>> in powersave mode. >>> Can you send me the output of lspci -vvvxxx please? >>> >> Here it is. Please note that I'm using pcie_aspm=force option. > Hi, Hi, Thank you very much for comments. > can you please split the patch into two patches, then I will have more detail > review/test. Also please don't do cleanup in the patches for new feature. Ok, I'll split the patch. I'm sorry but it would take a few days or more. > I had a simple look at the patch, and found at least one issue: > A link might have multiple child links, we need check all child links's latency to > enable the parent's ASPM. In pcie_config_aspm_path(), the patch seems not check > peer link. I'll check it. Thanks, Kenji Kaneshige > > Thanks, > Shaohua > -- > 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 > > -- 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
Shaohua Li wrote: > On Fri, Jun 19, 2009 at 01:28:44PM +0800, Kenji Kaneshige wrote: >> Shaohua Li wrote: >>> On Fri, Jun 19, 2009 at 11:19:49AM +0800, Kenji Kaneshige wrote: >>>> Shaohua Li wrote: >>>>> On Fri, Jun 19, 2009 at 05:28:41AM +0800, Jesse Barnes wrote: >>>>>> On Thu, 21 May 2009 11:10:06 +0900 >>>>>> Kenji Kaneshige <kaneshige.kenji@jp.fujitsu.com> wrote: >>>>>> >>>>>>> Hi, >>>>>>> >>>>>>> This patch changes ASPM driver to enable more detailed ASPM >>>>>>> configuration. With this patch, >>>>>>> >>>>>>> - ASPM can be enabled partially in the hierarchy. >>>>>>> - L0s can be managed for each direction (upstream direction and >>>>>>> downstream direction) of the link. >>>>> Well, these are nice features, but what kind of machine does you test? >>>>> Does it have several level hierarchy? The most powerful system I saw >>>>> just has two levels, which sounds not worth adding partial aspm. >>>> Thank you very much for comments. >>>> >>>> I think it is useful also for the system that doesn't have so >>>> many levels of hierarchy, such as mobile systems. Here is "lspci -t" >>>> output on my environment with some comments. My system also has >>>> just two levels, but with the patch, upstream L0s is enabled on the >>>> link 0000:15.00.0 (switch downstream port) to 0000:16:00.0 (endpoint) >>>> in powersave mode. >>> Can you send me the output of lspci -vvvxxx please? >>> >> Here it is. Please note that I'm using pcie_aspm=force option. > Hi, > can you please split the patch into two patches, then I will have more detail > review/test. Also please don't do cleanup in the patches for new feature. > I had a simple look at the patch, and found at least one issue: > A link might have multiple child links, we need check all child links's latency to > enable the parent's ASPM. In pcie_config_aspm_path(), the patch seems not check > peer link. > My understanding is that we can enable parent ASPM even if one or more child links don't meet the latency requirement and ASPM is disabled on them. In this case, parent link will never enter the upstream direction L0s nor L1 state because parent link's downstream component (switch upstream port) never becomes idle. Thanks, Kenji Kaneshige -- 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
On Fri, Jun 26, 2009 at 10:59:20AM +0800, Kenji Kaneshige wrote: > Shaohua Li wrote: > > On Fri, Jun 19, 2009 at 01:28:44PM +0800, Kenji Kaneshige wrote: > >> Shaohua Li wrote: > >>> On Fri, Jun 19, 2009 at 11:19:49AM +0800, Kenji Kaneshige wrote: > >>>> Shaohua Li wrote: > >>>>> On Fri, Jun 19, 2009 at 05:28:41AM +0800, Jesse Barnes wrote: > >>>>>> On Thu, 21 May 2009 11:10:06 +0900 > >>>>>> Kenji Kaneshige <kaneshige.kenji@jp.fujitsu.com> wrote: > >>>>>> > >>>>>>> Hi, > >>>>>>> > >>>>>>> This patch changes ASPM driver to enable more detailed ASPM > >>>>>>> configuration. With this patch, > >>>>>>> > >>>>>>> - ASPM can be enabled partially in the hierarchy. > >>>>>>> - L0s can be managed for each direction (upstream direction and > >>>>>>> downstream direction) of the link. > >>>>> Well, these are nice features, but what kind of machine does you test? > >>>>> Does it have several level hierarchy? The most powerful system I saw > >>>>> just has two levels, which sounds not worth adding partial aspm. > >>>> Thank you very much for comments. > >>>> > >>>> I think it is useful also for the system that doesn't have so > >>>> many levels of hierarchy, such as mobile systems. Here is "lspci -t" > >>>> output on my environment with some comments. My system also has > >>>> just two levels, but with the patch, upstream L0s is enabled on the > >>>> link 0000:15.00.0 (switch downstream port) to 0000:16:00.0 (endpoint) > >>>> in powersave mode. > >>> Can you send me the output of lspci -vvvxxx please? > >>> > >> Here it is. Please note that I'm using pcie_aspm=force option. > > Hi, > > can you please split the patch into two patches, then I will have more detail > > review/test. Also please don't do cleanup in the patches for new feature. > > I had a simple look at the patch, and found at least one issue: > > A link might have multiple child links, we need check all child links's latency to > > enable the parent's ASPM. In pcie_config_aspm_path(), the patch seems not check > > peer link. > > > > My understanding is that we can enable parent ASPM even if one > or more child links don't meet the latency requirement and ASPM > is disabled on them. In this case, parent link will never enter > the upstream direction L0s nor L1 state because parent link's > downstream component (switch upstream port) never becomes idle. Not sure about this. Microsoft's sides say switch's upstream link's ASPM is always enabled in vista, which seems like what you said. Thanks, Shaohua -- 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
Shaohua Li wrote: > On Fri, Jun 26, 2009 at 10:59:20AM +0800, Kenji Kaneshige wrote: >> Shaohua Li wrote: >>> On Fri, Jun 19, 2009 at 01:28:44PM +0800, Kenji Kaneshige wrote: >>>> Shaohua Li wrote: >>>>> On Fri, Jun 19, 2009 at 11:19:49AM +0800, Kenji Kaneshige wrote: >>>>>> Shaohua Li wrote: >>>>>>> On Fri, Jun 19, 2009 at 05:28:41AM +0800, Jesse Barnes wrote: >>>>>>>> On Thu, 21 May 2009 11:10:06 +0900 >>>>>>>> Kenji Kaneshige <kaneshige.kenji@jp.fujitsu.com> wrote: >>>>>>>> >>>>>>>>> Hi, >>>>>>>>> >>>>>>>>> This patch changes ASPM driver to enable more detailed ASPM >>>>>>>>> configuration. With this patch, >>>>>>>>> >>>>>>>>> - ASPM can be enabled partially in the hierarchy. >>>>>>>>> - L0s can be managed for each direction (upstream direction and >>>>>>>>> downstream direction) of the link. >>>>>>> Well, these are nice features, but what kind of machine does you test? >>>>>>> Does it have several level hierarchy? The most powerful system I saw >>>>>>> just has two levels, which sounds not worth adding partial aspm. >>>>>> Thank you very much for comments. >>>>>> >>>>>> I think it is useful also for the system that doesn't have so >>>>>> many levels of hierarchy, such as mobile systems. Here is "lspci -t" >>>>>> output on my environment with some comments. My system also has >>>>>> just two levels, but with the patch, upstream L0s is enabled on the >>>>>> link 0000:15.00.0 (switch downstream port) to 0000:16:00.0 (endpoint) >>>>>> in powersave mode. >>>>> Can you send me the output of lspci -vvvxxx please? >>>>> >>>> Here it is. Please note that I'm using pcie_aspm=force option. >>> Hi, >>> can you please split the patch into two patches, then I will have more detail >>> review/test. Also please don't do cleanup in the patches for new feature. >>> I had a simple look at the patch, and found at least one issue: >>> A link might have multiple child links, we need check all child links's latency to >>> enable the parent's ASPM. In pcie_config_aspm_path(), the patch seems not check >>> peer link. >>> >> My understanding is that we can enable parent ASPM even if one >> or more child links don't meet the latency requirement and ASPM >> is disabled on them. In this case, parent link will never enter >> the upstream direction L0s nor L1 state because parent link's >> downstream component (switch upstream port) never becomes idle. > Not sure about this. Microsoft's sides say switch's upstream link's > ASPM is always enabled in vista, which seems like what you said. > My understanding about this is from page 339 for L0s and page 342 for L1 of PCI Express spec 2.1. By the way, though I don't know Microsoft's implementation, if it always enables ASPM on switch's upstream link, the algorithm is different from my patch. My patch disables ASPM on parent link if it doesn't meet the latency requirement. If we always enable ASPM on parent link, we cannot enable ASPM on the child link in the following situation. - parent link doesn't meet latency requirement - child link meets latency requirement. Thanks, Kenji Kaneshige -- 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
On Fri, Jun 26, 2009 at 10:59:20AM +0800, Kenji Kaneshige wrote: > Shaohua Li wrote: > > On Fri, Jun 19, 2009 at 01:28:44PM +0800, Kenji Kaneshige wrote: > >> Shaohua Li wrote: > >>> On Fri, Jun 19, 2009 at 11:19:49AM +0800, Kenji Kaneshige wrote: > >>>> Shaohua Li wrote: > >>>>> On Fri, Jun 19, 2009 at 05:28:41AM +0800, Jesse Barnes wrote: > >>>>>> On Thu, 21 May 2009 11:10:06 +0900 > >>>>>> Kenji Kaneshige <kaneshige.kenji@jp.fujitsu.com> wrote: > >>>>>> > >>>>>>> Hi, > >>>>>>> > >>>>>>> This patch changes ASPM driver to enable more detailed ASPM > >>>>>>> configuration. With this patch, > >>>>>>> > >>>>>>> - ASPM can be enabled partially in the hierarchy. > >>>>>>> - L0s can be managed for each direction (upstream direction and > >>>>>>> downstream direction) of the link. > >>>>> Well, these are nice features, but what kind of machine does you test? > >>>>> Does it have several level hierarchy? The most powerful system I saw > >>>>> just has two levels, which sounds not worth adding partial aspm. > >>>> Thank you very much for comments. > >>>> > >>>> I think it is useful also for the system that doesn't have so > >>>> many levels of hierarchy, such as mobile systems. Here is "lspci -t" > >>>> output on my environment with some comments. My system also has > >>>> just two levels, but with the patch, upstream L0s is enabled on the > >>>> link 0000:15.00.0 (switch downstream port) to 0000:16:00.0 (endpoint) > >>>> in powersave mode. > >>> Can you send me the output of lspci -vvvxxx please? > >>> > >> Here it is. Please note that I'm using pcie_aspm=force option. > > Hi, > > can you please split the patch into two patches, then I will have more detail > > review/test. Also please don't do cleanup in the patches for new feature. > > I had a simple look at the patch, and found at least one issue: > > A link might have multiple child links, we need check all child links's latency to > > enable the parent's ASPM. In pcie_config_aspm_path(), the patch seems not check > > peer link. > > > > My understanding is that we can enable parent ASPM even if one > or more child links don't meet the latency requirement and ASPM > is disabled on them. In this case, parent link will never enter > the upstream direction L0s nor L1 state because parent link's > downstream component (switch upstream port) never becomes idle. Yes, for the case you pointed out, not enabling parent link is always better. Let's take this way: 1. always enable downstream's L0s (root port or switch downstream port) if downstream device supports L0s. 2. like what you said, enabling endpoint's link if its link meets latency requirement but its parent link doesn't. what do you think? Thanks, Shaohua -- 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
On Fri, Jun 26, 2009 at 10:59:20AM +0800, Kenji Kaneshige wrote: > Shaohua Li wrote: > > On Fri, Jun 19, 2009 at 01:28:44PM +0800, Kenji Kaneshige wrote: > >> Shaohua Li wrote: > >>> On Fri, Jun 19, 2009 at 11:19:49AM +0800, Kenji Kaneshige wrote: > >>>> Shaohua Li wrote: > >>>>> On Fri, Jun 19, 2009 at 05:28:41AM +0800, Jesse Barnes wrote: > >>>>>> On Thu, 21 May 2009 11:10:06 +0900 > >>>>>> Kenji Kaneshige <kaneshige.kenji@jp.fujitsu.com> wrote: > >>>>>> > >>>>>>> Hi, > >>>>>>> > >>>>>>> This patch changes ASPM driver to enable more detailed ASPM > >>>>>>> configuration. With this patch, > >>>>>>> > >>>>>>> - ASPM can be enabled partially in the hierarchy. > >>>>>>> - L0s can be managed for each direction (upstream direction and > >>>>>>> downstream direction) of the link. > >>>>> Well, these are nice features, but what kind of machine does you test? > >>>>> Does it have several level hierarchy? The most powerful system I saw > >>>>> just has two levels, which sounds not worth adding partial aspm. > >>>> Thank you very much for comments. > >>>> > >>>> I think it is useful also for the system that doesn't have so > >>>> many levels of hierarchy, such as mobile systems. Here is "lspci -t" > >>>> output on my environment with some comments. My system also has > >>>> just two levels, but with the patch, upstream L0s is enabled on the > >>>> link 0000:15.00.0 (switch downstream port) to 0000:16:00.0 (endpoint) > >>>> in powersave mode. > >>> Can you send me the output of lspci -vvvxxx please? > >>> > >> Here it is. Please note that I'm using pcie_aspm=force option. > > Hi, > > can you please split the patch into two patches, then I will have more detail > > review/test. Also please don't do cleanup in the patches for new feature. > > I had a simple look at the patch, and found at least one issue: > > A link might have multiple child links, we need check all child links's latency to > > enable the parent's ASPM. In pcie_config_aspm_path(), the patch seems not check > > peer link. > > > > My understanding is that we can enable parent ASPM even if one > or more child links don't meet the latency requirement and ASPM > is disabled on them. In this case, parent link will never enter > the upstream direction L0s nor L1 state because parent link's > downstream component (switch upstream port) never becomes idle. I thought we need to check other links, considering below case: Say we have three links, parent/child1/child2. When you do partial aspm enabling, endpoints in child1 have latency requirement and the result is child1 is enabled and parent is disabled. The we check child2, and latency requirement meets and then you enable child2 and parent. This will makes endpoints in link1 hasn't required latency and breaks them. Thanks, Shaohua -- 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
Shaohua Li wrote: > On Fri, Jun 26, 2009 at 10:59:20AM +0800, Kenji Kaneshige wrote: >> Shaohua Li wrote: >>> On Fri, Jun 19, 2009 at 01:28:44PM +0800, Kenji Kaneshige wrote: >>>> Shaohua Li wrote: >>>>> On Fri, Jun 19, 2009 at 11:19:49AM +0800, Kenji Kaneshige wrote: >>>>>> Shaohua Li wrote: >>>>>>> On Fri, Jun 19, 2009 at 05:28:41AM +0800, Jesse Barnes wrote: >>>>>>>> On Thu, 21 May 2009 11:10:06 +0900 >>>>>>>> Kenji Kaneshige <kaneshige.kenji@jp.fujitsu.com> wrote: >>>>>>>> >>>>>>>>> Hi, >>>>>>>>> >>>>>>>>> This patch changes ASPM driver to enable more detailed ASPM >>>>>>>>> configuration. With this patch, >>>>>>>>> >>>>>>>>> - ASPM can be enabled partially in the hierarchy. >>>>>>>>> - L0s can be managed for each direction (upstream direction and >>>>>>>>> downstream direction) of the link. >>>>>>> Well, these are nice features, but what kind of machine does you test? >>>>>>> Does it have several level hierarchy? The most powerful system I saw >>>>>>> just has two levels, which sounds not worth adding partial aspm. >>>>>> Thank you very much for comments. >>>>>> >>>>>> I think it is useful also for the system that doesn't have so >>>>>> many levels of hierarchy, such as mobile systems. Here is "lspci -t" >>>>>> output on my environment with some comments. My system also has >>>>>> just two levels, but with the patch, upstream L0s is enabled on the >>>>>> link 0000:15.00.0 (switch downstream port) to 0000:16:00.0 (endpoint) >>>>>> in powersave mode. >>>>> Can you send me the output of lspci -vvvxxx please? >>>>> >>>> Here it is. Please note that I'm using pcie_aspm=force option. >>> Hi, >>> can you please split the patch into two patches, then I will have more detail >>> review/test. Also please don't do cleanup in the patches for new feature. >>> I had a simple look at the patch, and found at least one issue: >>> A link might have multiple child links, we need check all child links's latency to >>> enable the parent's ASPM. In pcie_config_aspm_path(), the patch seems not check >>> peer link. >>> >> My understanding is that we can enable parent ASPM even if one >> or more child links don't meet the latency requirement and ASPM >> is disabled on them. In this case, parent link will never enter >> the upstream direction L0s nor L1 state because parent link's >> downstream component (switch upstream port) never becomes idle. > Yes, for the case you pointed out, not enabling parent link is always better. Why do you think it's better? Doing this (not enabling parent link) would needs complicated handling and increase the cost very much. > Let's take this way: > 1. always enable downstream's L0s (root port or switch downstream port) > if downstream device supports L0s. If we always enable L0s on downstream port, downstream direction L0s will be always enabled. Do you mean we don't need to check latency for downstream direction L0s? > 2. like what you said, enabling endpoint's link if its link meets latency > requirement but its parent link doesn't. I'm sorry but I do not understand the point of your suggestion. What is the benefit? What do you think about switch upstream ports? By the way, I'm very sorry for the delay of sending separated patches. I've already separated the patch into several patches and compile test has been done. But those are not tested one by one yet. If it's OK for you, I'll send them before testing. Thanks, Kenji Kaneshige -- 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
Shaohua Li wrote: > On Fri, Jun 26, 2009 at 10:59:20AM +0800, Kenji Kaneshige wrote: >> Shaohua Li wrote: >>> On Fri, Jun 19, 2009 at 01:28:44PM +0800, Kenji Kaneshige wrote: >>>> Shaohua Li wrote: >>>>> On Fri, Jun 19, 2009 at 11:19:49AM +0800, Kenji Kaneshige wrote: >>>>>> Shaohua Li wrote: >>>>>>> On Fri, Jun 19, 2009 at 05:28:41AM +0800, Jesse Barnes wrote: >>>>>>>> On Thu, 21 May 2009 11:10:06 +0900 >>>>>>>> Kenji Kaneshige <kaneshige.kenji@jp.fujitsu.com> wrote: >>>>>>>> >>>>>>>>> Hi, >>>>>>>>> >>>>>>>>> This patch changes ASPM driver to enable more detailed ASPM >>>>>>>>> configuration. With this patch, >>>>>>>>> >>>>>>>>> - ASPM can be enabled partially in the hierarchy. >>>>>>>>> - L0s can be managed for each direction (upstream direction and >>>>>>>>> downstream direction) of the link. >>>>>>> Well, these are nice features, but what kind of machine does you test? >>>>>>> Does it have several level hierarchy? The most powerful system I saw >>>>>>> just has two levels, which sounds not worth adding partial aspm. >>>>>> Thank you very much for comments. >>>>>> >>>>>> I think it is useful also for the system that doesn't have so >>>>>> many levels of hierarchy, such as mobile systems. Here is "lspci -t" >>>>>> output on my environment with some comments. My system also has >>>>>> just two levels, but with the patch, upstream L0s is enabled on the >>>>>> link 0000:15.00.0 (switch downstream port) to 0000:16:00.0 (endpoint) >>>>>> in powersave mode. >>>>> Can you send me the output of lspci -vvvxxx please? >>>>> >>>> Here it is. Please note that I'm using pcie_aspm=force option. >>> Hi, >>> can you please split the patch into two patches, then I will have more detail >>> review/test. Also please don't do cleanup in the patches for new feature. >>> I had a simple look at the patch, and found at least one issue: >>> A link might have multiple child links, we need check all child links's latency to >>> enable the parent's ASPM. In pcie_config_aspm_path(), the patch seems not check >>> peer link. >>> >> My understanding is that we can enable parent ASPM even if one >> or more child links don't meet the latency requirement and ASPM >> is disabled on them. In this case, parent link will never enter >> the upstream direction L0s nor L1 state because parent link's >> downstream component (switch upstream port) never becomes idle. > I thought we need to check other links, considering below case: > Say we have three links, parent/child1/child2. When you do partial aspm > enabling, endpoints in child1 have latency requirement and the result is child1 > is enabled and parent is disabled. The we check child2, and latency requirement > meets and then you enable child2 and parent. This will makes endpoints in link1 > hasn't required latency and breaks them. > In this case, my patch doesn't enable ASPM on parent the link. My patch is doing as follows: 1) Detect parent link and initialize 'aspm_support'(*1) and 'aspm_capable'(*1). At this point 'aspm_capable' is equal to 'aspm_support'(*2) because no endpoints are detected at this point. And enable ASPM based on 'aspm_capable' on parent. The states are as follows at this point (note that all ASPM states are mixed up in this example). parent: Support+, Capable+, Enabled+ child1: N/A , N/A , N/A child2: N/A , N/A , N/A (*1): Each bits in 'aspm_capable' is associated to upstream and downstream direction L0s and L1. The bit is set if the link supports associated ASPM state and meets latency requirement. (*2): Each bits in 'aspm_support' is associated to upstream and downstream direction L0s and L1. The bit is set if the link supports associated ASPM state. 2) Detect child1 link and initialize 'aspm_support' and 'aspm_capable'. First, 'aspm_capable' is initialized by 'aspm_support' and then updated based on endpoint's acceptable latency. The parent's 'aspm_capable' is also updated here. In this update, bits can be unset, but never be set. In your example, 'aspm_capable' is unset because it doesn't meets latency requirement. And then configure ASPM based on 'aspm_capable' on child1 and parent. The states are as follows at this point. parent: Support+, Capable-, Enabled- child1: Support+, Capable+, Enabled+ child2: N/A , N/A , N/A 3) Detect child2 link and initialize 'aspm_support' and 'aspm_capable'. First, 'aspm_capable' is initialized by 'aspm_support' and then updated based on endpoint's acceptable latency. The parent's 'aspm_capable' is also updated here, but bits are never set in this update. In your example, parent meets latency requirement for endpoint under child2 link, but 'aspm_capable' of parent is never changed. And then configure ASPM based on 'aspm_capable' on child2 and parent. The states are as follows at the end. parent: Support+, Capable-, Enabled- child1: Support+, Capable+, Enabled+ child2: Support+, Capable+, Enabled+ Thanks, Kenji Kaneshige -- 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
On Mon, Jun 29, 2009 at 04:49:10PM +0800, Kenji Kaneshige wrote: > Shaohua Li wrote: > > On Fri, Jun 26, 2009 at 10:59:20AM +0800, Kenji Kaneshige wrote: > >> Shaohua Li wrote: > >>> On Fri, Jun 19, 2009 at 01:28:44PM +0800, Kenji Kaneshige wrote: > >>>> Shaohua Li wrote: > >>>>> On Fri, Jun 19, 2009 at 11:19:49AM +0800, Kenji Kaneshige wrote: > >>>>>> Shaohua Li wrote: > >>>>>>> On Fri, Jun 19, 2009 at 05:28:41AM +0800, Jesse Barnes wrote: > >>>>>>>> On Thu, 21 May 2009 11:10:06 +0900 > >>>>>>>> Kenji Kaneshige <kaneshige.kenji@jp.fujitsu.com> wrote: > >>>>>>>> > >>>>>>>>> Hi, > >>>>>>>>> > >>>>>>>>> This patch changes ASPM driver to enable more detailed ASPM > >>>>>>>>> configuration. With this patch, > >>>>>>>>> > >>>>>>>>> - ASPM can be enabled partially in the hierarchy. > >>>>>>>>> - L0s can be managed for each direction (upstream direction and > >>>>>>>>> downstream direction) of the link. > >>>>>>> Well, these are nice features, but what kind of machine does you test? > >>>>>>> Does it have several level hierarchy? The most powerful system I saw > >>>>>>> just has two levels, which sounds not worth adding partial aspm. > >>>>>> Thank you very much for comments. > >>>>>> > >>>>>> I think it is useful also for the system that doesn't have so > >>>>>> many levels of hierarchy, such as mobile systems. Here is "lspci -t" > >>>>>> output on my environment with some comments. My system also has > >>>>>> just two levels, but with the patch, upstream L0s is enabled on the > >>>>>> link 0000:15.00.0 (switch downstream port) to 0000:16:00.0 (endpoint) > >>>>>> in powersave mode. > >>>>> Can you send me the output of lspci -vvvxxx please? > >>>>> > >>>> Here it is. Please note that I'm using pcie_aspm=force option. > >>> Hi, > >>> can you please split the patch into two patches, then I will have more detail > >>> review/test. Also please don't do cleanup in the patches for new feature. > >>> I had a simple look at the patch, and found at least one issue: > >>> A link might have multiple child links, we need check all child links's latency to > >>> enable the parent's ASPM. In pcie_config_aspm_path(), the patch seems not check > >>> peer link. > >>> > >> My understanding is that we can enable parent ASPM even if one > >> or more child links don't meet the latency requirement and ASPM > >> is disabled on them. In this case, parent link will never enter > >> the upstream direction L0s nor L1 state because parent link's > >> downstream component (switch upstream port) never becomes idle. > > I thought we need to check other links, considering below case: > > Say we have three links, parent/child1/child2. When you do partial aspm > > enabling, endpoints in child1 have latency requirement and the result is child1 > > is enabled and parent is disabled. The we check child2, and latency requirement > > meets and then you enable child2 and parent. This will makes endpoints in link1 > > hasn't required latency and breaks them. > > > > In this case, my patch doesn't enable ASPM on parent the link. > > My patch is doing as follows: > > 1) Detect parent link and initialize 'aspm_support'(*1) and > 'aspm_capable'(*1). At this point 'aspm_capable' is equal to > 'aspm_support'(*2) because no endpoints are detected at this > point. And enable ASPM based on 'aspm_capable' on parent. > > The states are as follows at this point (note that all ASPM > states are mixed up in this example). > > parent: Support+, Capable+, Enabled+ > child1: N/A , N/A , N/A > child2: N/A , N/A , N/A > > (*1): Each bits in 'aspm_capable' is associated to upstream and > downstream direction L0s and L1. The bit is set if the link > supports associated ASPM state and meets latency requirement. > > (*2): Each bits in 'aspm_support' is associated to upstream and > downstream direction L0s and L1. The bit is set if the link > supports associated ASPM state. > > > 2) Detect child1 link and initialize 'aspm_support' and 'aspm_capable'. > First, 'aspm_capable' is initialized by 'aspm_support' and then > updated based on endpoint's acceptable latency. The parent's > 'aspm_capable' is also updated here. In this update, bits can be > unset, but never be set. In your example, 'aspm_capable' > is unset because it doesn't meets latency requirement. And then > configure ASPM based on 'aspm_capable' on child1 and parent. > > The states are as follows at this point. > > parent: Support+, Capable-, Enabled- > child1: Support+, Capable+, Enabled+ > child2: N/A , N/A , N/A > > 3) Detect child2 link and initialize 'aspm_support' and 'aspm_capable'. > First, 'aspm_capable' is initialized by 'aspm_support' and then > updated based on endpoint's acceptable latency. The parent's > 'aspm_capable' is also updated here, but bits are never set in this > update. In your example, parent meets latency requirement for > endpoint under child2 link, but 'aspm_capable' of parent is never > changed. And then configure ASPM based on 'aspm_capable' on child2 > and parent. > > The states are as follows at the end. > > parent: Support+, Capable-, Enabled- > child1: Support+, Capable+, Enabled+ > child2: Support+, Capable+, Enabled+ Ok, you are right, I missed you set capable -. Thanks, Shaohua -- 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
On Mon, Jun 29, 2009 at 04:17:12PM +0800, Kenji Kaneshige wrote: > Shaohua Li wrote: > > On Fri, Jun 26, 2009 at 10:59:20AM +0800, Kenji Kaneshige wrote: > >> Shaohua Li wrote: > >>> On Fri, Jun 19, 2009 at 01:28:44PM +0800, Kenji Kaneshige wrote: > >>>> Shaohua Li wrote: > >>>>> On Fri, Jun 19, 2009 at 11:19:49AM +0800, Kenji Kaneshige wrote: > >>>>>> Shaohua Li wrote: > >>>>>>> On Fri, Jun 19, 2009 at 05:28:41AM +0800, Jesse Barnes wrote: > >>>>>>>> On Thu, 21 May 2009 11:10:06 +0900 > >>>>>>>> Kenji Kaneshige <kaneshige.kenji@jp.fujitsu.com> wrote: > >>>>>>>> > >>>>>>>>> Hi, > >>>>>>>>> > >>>>>>>>> This patch changes ASPM driver to enable more detailed ASPM > >>>>>>>>> configuration. With this patch, > >>>>>>>>> > >>>>>>>>> - ASPM can be enabled partially in the hierarchy. > >>>>>>>>> - L0s can be managed for each direction (upstream direction and > >>>>>>>>> downstream direction) of the link. > >>>>>>> Well, these are nice features, but what kind of machine does you test? > >>>>>>> Does it have several level hierarchy? The most powerful system I saw > >>>>>>> just has two levels, which sounds not worth adding partial aspm. > >>>>>> Thank you very much for comments. > >>>>>> > >>>>>> I think it is useful also for the system that doesn't have so > >>>>>> many levels of hierarchy, such as mobile systems. Here is "lspci -t" > >>>>>> output on my environment with some comments. My system also has > >>>>>> just two levels, but with the patch, upstream L0s is enabled on the > >>>>>> link 0000:15.00.0 (switch downstream port) to 0000:16:00.0 (endpoint) > >>>>>> in powersave mode. > >>>>> Can you send me the output of lspci -vvvxxx please? > >>>>> > >>>> Here it is. Please note that I'm using pcie_aspm=force option. > >>> Hi, > >>> can you please split the patch into two patches, then I will have more detail > >>> review/test. Also please don't do cleanup in the patches for new feature. > >>> I had a simple look at the patch, and found at least one issue: > >>> A link might have multiple child links, we need check all child links's latency to > >>> enable the parent's ASPM. In pcie_config_aspm_path(), the patch seems not check > >>> peer link. > >>> > >> My understanding is that we can enable parent ASPM even if one > >> or more child links don't meet the latency requirement and ASPM > >> is disabled on them. In this case, parent link will never enter > >> the upstream direction L0s nor L1 state because parent link's > >> downstream component (switch upstream port) never becomes idle. > > Yes, for the case you pointed out, not enabling parent link is always better. > > Why do you think it's better? Because even parent link is enabled, if its child can't enable aspm, the parent link still can't enter aspm. And if parent link isn't enabled, child link can enable aspm. > Doing this (not enabling parent link) would needs complicated > handling and increase the cost very much. yes, it's complicated, but worthy. I thought your partial aspm enabling already handles it well, right? > > Let's take this way: > > 1. always enable downstream's L0s (root port or switch downstream port) > > if downstream device supports L0s. > > If we always enable L0s on downstream port, downstream direction > L0s will be always enabled. Do you mean we don't need to check > latency for downstream direction L0s? please ignore this, we still need check the latency. > > 2. like what you said, enabling endpoint's link if its link meets latency > > requirement but its parent link doesn't. > > I'm sorry but I do not understand the point of your suggestion. > What is the benefit? What do you think about switch upstream ports? Just think your solution is better than always enabling parent link's aspm. > By the way, I'm very sorry for the delay of sending separated > patches. I've already separated the patch into several patches > and compile test has been done. But those are not tested one by > one yet. If it's OK for you, I'll send them before testing. Don't worry about it, not urgent. Thanks, Shaohua -- 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
Shaohua Li wrote: > On Mon, Jun 29, 2009 at 04:17:12PM +0800, Kenji Kaneshige wrote: >> Shaohua Li wrote: >>> On Fri, Jun 26, 2009 at 10:59:20AM +0800, Kenji Kaneshige wrote: >>>> Shaohua Li wrote: >>>>> On Fri, Jun 19, 2009 at 01:28:44PM +0800, Kenji Kaneshige wrote: >>>>>> Shaohua Li wrote: >>>>>>> On Fri, Jun 19, 2009 at 11:19:49AM +0800, Kenji Kaneshige wrote: >>>>>>>> Shaohua Li wrote: >>>>>>>>> On Fri, Jun 19, 2009 at 05:28:41AM +0800, Jesse Barnes wrote: >>>>>>>>>> On Thu, 21 May 2009 11:10:06 +0900 >>>>>>>>>> Kenji Kaneshige <kaneshige.kenji@jp.fujitsu.com> wrote: >>>>>>>>>> >>>>>>>>>>> Hi, >>>>>>>>>>> >>>>>>>>>>> This patch changes ASPM driver to enable more detailed ASPM >>>>>>>>>>> configuration. With this patch, >>>>>>>>>>> >>>>>>>>>>> - ASPM can be enabled partially in the hierarchy. >>>>>>>>>>> - L0s can be managed for each direction (upstream direction and >>>>>>>>>>> downstream direction) of the link. >>>>>>>>> Well, these are nice features, but what kind of machine does you test? >>>>>>>>> Does it have several level hierarchy? The most powerful system I saw >>>>>>>>> just has two levels, which sounds not worth adding partial aspm. >>>>>>>> Thank you very much for comments. >>>>>>>> >>>>>>>> I think it is useful also for the system that doesn't have so >>>>>>>> many levels of hierarchy, such as mobile systems. Here is "lspci -t" >>>>>>>> output on my environment with some comments. My system also has >>>>>>>> just two levels, but with the patch, upstream L0s is enabled on the >>>>>>>> link 0000:15.00.0 (switch downstream port) to 0000:16:00.0 (endpoint) >>>>>>>> in powersave mode. >>>>>>> Can you send me the output of lspci -vvvxxx please? >>>>>>> >>>>>> Here it is. Please note that I'm using pcie_aspm=force option. >>>>> Hi, >>>>> can you please split the patch into two patches, then I will have more detail >>>>> review/test. Also please don't do cleanup in the patches for new feature. >>>>> I had a simple look at the patch, and found at least one issue: >>>>> A link might have multiple child links, we need check all child links's latency to >>>>> enable the parent's ASPM. In pcie_config_aspm_path(), the patch seems not check >>>>> peer link. >>>>> >>>> My understanding is that we can enable parent ASPM even if one >>>> or more child links don't meet the latency requirement and ASPM >>>> is disabled on them. In this case, parent link will never enter >>>> the upstream direction L0s nor L1 state because parent link's >>>> downstream component (switch upstream port) never becomes idle. >>> Yes, for the case you pointed out, not enabling parent link is always better. >> Why do you think it's better? > Because even parent link is enabled, if its child can't enable aspm, the parent > link still can't enter aspm. And if parent link isn't enabled, child link can > enable aspm. Oh, I totally misunderstood what you meant. Sorry. I thought you're saying that we should disable ASPM on parent link whenever ASPM is disabled on its one or more child links. Yes, what you said is the same as my understanding. > >> Doing this (not enabling parent link) would needs complicated >> handling and increase the cost very much. > yes, it's complicated, but worthy. I thought your partial aspm enabling already > handles it well, right? Yes, my patch is already handles it (if child link meets latency requirement and the parent link doesn't, disable ASPM on parent link and enable ASPM on child link). > >>> Let's take this way: >>> 1. always enable downstream's L0s (root port or switch downstream port) >>> if downstream device supports L0s. >> If we always enable L0s on downstream port, downstream direction >> L0s will be always enabled. Do you mean we don't need to check >> latency for downstream direction L0s? > please ignore this, we still need check the latency. > >>> 2. like what you said, enabling endpoint's link if its link meets latency >>> requirement but its parent link doesn't. >> I'm sorry but I do not understand the point of your suggestion. >> What is the benefit? What do you think about switch upstream ports? > Just think your solution is better than always enabling parent link's aspm. Ok, thank you. > >> By the way, I'm very sorry for the delay of sending separated >> patches. I've already separated the patch into several patches >> and compile test has been done. But those are not tested one by >> one yet. If it's OK for you, I'll send them before testing. > Don't worry about it, not urgent. > Thank you, I think I can send the updated patches in this week. Thanks, Kenji Kaneshige -- 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
Index: 20090521/drivers/pci/pcie/aspm.c =================================================================== --- 20090521.orig/drivers/pci/pcie/aspm.c +++ 20090521/drivers/pci/pcie/aspm.c @@ -19,6 +19,8 @@ #include <linux/jiffies.h> #include <linux/delay.h> #include <linux/pci-aspm.h> +#include <linux/proc_fs.h> +#include <linux/seq_file.h> #include "../pci.h" #ifdef MODULE_PARAM_PREFIX @@ -859,6 +861,117 @@ void pcie_aspm_remove_sysfs_dev_files(st sysfs_remove_file_from_group(&pdev->dev.kobj, &dev_attr_clk_ctl.attr, power_group); } + +static void show_link(struct seq_file *p, + struct pcie_link_state *link, int depth) +{ + struct pci_dev *child, *parent = link->pdev; + struct pci_bus *linkbus = parent->subordinate; + struct pcie_link_state *childlink; + int indent = depth * 4; + + seq_printf(p, + "%*sLINK (addr: %p, pdev: %p, name: %s)\n" + "%*s root: %p, parent %p\n" + "%*s ASPM enabled: L0sUP%c, L0sDW%c, L1%c\n" + "%*s ASPM capable: L0sUP%c, L0sDW%c, L1%c\n" + "%*s ASPM support: L0sUP%c, L0sDW%c, L1%c\n" + "%*s ASPM disable: L0sUP%c, L0sDW%c, L1%c\n" + "%*s ASPM default: L0sUP%c, L0sDW%c, L1%c\n" + "%*s ASPM latency: L0sUP %u, L0sDW %u, L1 %u\n" + "%*s ClockPM: capable %u, enabled %u, default %u\n", + indent, "", link, parent, pci_name(parent), + indent, "", link->root, link->parent, + indent, "", + (link->aspm_enabled & ASPM_STATE_L0S_UP) ? '+' : '-', + (link->aspm_enabled & ASPM_STATE_L0S_DW) ? '+' : '-', + (link->aspm_enabled & ASPM_STATE_L1) ? '+' : '-', + indent, "", + (link->aspm_capable & ASPM_STATE_L0S_UP) ? '+' : '-', + (link->aspm_capable & ASPM_STATE_L0S_DW) ? '+' : '-', + (link->aspm_capable & ASPM_STATE_L1) ? '+' : '-', + indent, "", + (link->aspm_support & ASPM_STATE_L0S_UP) ? '+' : '-', + (link->aspm_support & ASPM_STATE_L0S_DW) ? '+' : '-', + (link->aspm_support & ASPM_STATE_L1) ? '+' : '-', + indent, "", + (link->aspm_disable & ASPM_STATE_L0S_UP) ? '+' : '-', + (link->aspm_disable & ASPM_STATE_L0S_DW) ? '+' : '-', + (link->aspm_disable & ASPM_STATE_L1) ? '+' : '-', + indent, "", + (link->aspm_default & ASPM_STATE_L0S_UP) ? '+' : '-', + (link->aspm_default & ASPM_STATE_L0S_DW) ? '+' : '-', + (link->aspm_default & ASPM_STATE_L1) ? '+' : '-', + indent, "", + link->latency_up.l0s, link->latency_dw.l0s, + max_t(u32, link->latency_up.l1, link->latency_dw.l1), + indent, "", link->clkpm_capable, + link->clkpm_enabled, link->clkpm_default); + list_for_each_entry(child, &linkbus->devices, bus_list) { + int func = PCI_FUNC(child->devfn); + if ((child->pcie_type != PCI_EXP_TYPE_ENDPOINT) && + (child->pcie_type != PCI_EXP_TYPE_LEG_END)) + continue; + seq_printf(p, "%*s EP[%s] accept l0s %u, l1 %u\n", + indent, "", pci_name(child), + link->acceptable[func].l0s, + link->acceptable[func].l1); + } + seq_printf(p, "\n"); + + list_for_each_entry(childlink, &link->children, link) + show_link(p, childlink, (depth + 1)); +} + +static int show_pcie_link_state(struct seq_file *p, void *v) +{ + struct pcie_link_state *link; + + mutex_lock(&aspm_lock); + list_for_each_entry(link, &link_list, sibling) { + if (!link->parent) + show_link(p, link, 0); + } + mutex_unlock(&aspm_lock); + return 0; +} + +static int pcie_link_state_open(struct inode *inode, struct file *file) +{ + size_t size = 128 * 1024; + char *buf; + int res; + struct seq_file *m; + + buf = kmalloc(size, GFP_KERNEL); + if (!buf) + return -ENOMEM; + + res = single_open(file, show_pcie_link_state, NULL); + if (!res) { + m = file->private_data; + m->buf = buf; + m->size = size; + } else + kfree(buf); + return res; +} + +static const struct file_operations pcie_link_state_operations = { + .open = pcie_link_state_open, + .read = seq_read, + .llseek = seq_lseek, + .release = single_release, +}; + +static int pcie_aspm_init_proc(void) +{ + if (aspm_disabled) + return 0; + proc_create("pcie_link_state", 0, NULL, &pcie_link_state_operations); + return 0; +} +late_initcall(pcie_aspm_init_proc); #endif static int __init pcie_aspm_disable(char *str)