* [PATCH] powerpc/eeh: Avoid to handle EEH on a passed Child PE @ 2015-09-21 9:29 Wei Yang 2015-09-21 11:49 ` Gavin Shan 0 siblings, 1 reply; 5+ messages in thread From: Wei Yang @ 2015-09-21 9:29 UTC (permalink / raw) To: gwshan; +Cc: linuxppc-dev, Wei Yang Current EEH infrastructure would avoid to handle EEH when a PE is passed to guest, while if this PE is a Child PE of the one hit EEH, host would handle this. By doing so, this would leads to guest hang. The correct way is avoid to handle it on host and let guest to recover. This patch avoids to handle EEH on a passed Child PE. Signed-off-by: Wei Yang <weiyang@linux.vnet.ibm.com> --- arch/powerpc/kernel/eeh_pe.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/arch/powerpc/kernel/eeh_pe.c b/arch/powerpc/kernel/eeh_pe.c index 5cde950..c6d0e9f 100644 --- a/arch/powerpc/kernel/eeh_pe.c +++ b/arch/powerpc/kernel/eeh_pe.c @@ -172,6 +172,7 @@ static struct eeh_pe *eeh_pe_next(struct eeh_pe *pe, * callback returns something other than NULL, or no more PEs * to be traversed. */ +static void *__eeh_pe_get(void *data, void *flag); void *eeh_pe_traverse(struct eeh_pe *root, eeh_traverse_func fn, void *flag) { @@ -179,6 +180,8 @@ void *eeh_pe_traverse(struct eeh_pe *root, void *ret; for (pe = root; pe; pe = eeh_pe_next(pe, root)) { + if (eeh_pe_passed(pe) && (fn != __eeh_pe_get)) + continue; ret = fn(pe, flag); if (ret) return ret; } @@ -210,6 +213,8 @@ void *eeh_pe_dev_traverse(struct eeh_pe *root, /* Traverse root PE */ for (pe = root; pe; pe = eeh_pe_next(pe, root)) { + if (eeh_pe_passed(pe)) + continue; eeh_pe_for_each_dev(pe, edev, tmp) { ret = fn(edev, flag); if (ret) -- 2.5.0 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] powerpc/eeh: Avoid to handle EEH on a passed Child PE 2015-09-21 9:29 [PATCH] powerpc/eeh: Avoid to handle EEH on a passed Child PE Wei Yang @ 2015-09-21 11:49 ` Gavin Shan 2015-09-22 4:43 ` Wei Yang 0 siblings, 1 reply; 5+ messages in thread From: Gavin Shan @ 2015-09-21 11:49 UTC (permalink / raw) To: Wei Yang; +Cc: gwshan, linuxppc-dev On Mon, Sep 21, 2015 at 05:29:48PM +0800, Wei Yang wrote: >Current EEH infrastructure would avoid to handle EEH when a PE is passed to >guest, while if this PE is a Child PE of the one hit EEH, host would handle >this. By doing so, this would leads to guest hang. The correct way is >avoid to handle it on host and let guest to recover. > >This patch avoids to handle EEH on a passed Child PE. > Ok. It's fixing the problem the guest, which owns a VF, when its PF hitting EEH error, right? If so, I'm not sure if you really tested this code. Does it work for you? When the parent PE (PF) is stopped for EEH recovery, it sounds impossible that the child PE can't be affected and just escape from the error. The question is how the guest can continue to work after the EEH recovery on parent PE? >Signed-off-by: Wei Yang <weiyang@linux.vnet.ibm.com> >--- > arch/powerpc/kernel/eeh_pe.c | 5 +++++ > 1 file changed, 5 insertions(+) > >diff --git a/arch/powerpc/kernel/eeh_pe.c b/arch/powerpc/kernel/eeh_pe.c >index 5cde950..c6d0e9f 100644 >--- a/arch/powerpc/kernel/eeh_pe.c >+++ b/arch/powerpc/kernel/eeh_pe.c >@@ -172,6 +172,7 @@ static struct eeh_pe *eeh_pe_next(struct eeh_pe *pe, > * callback returns something other than NULL, or no more PEs > * to be traversed. > */ >+static void *__eeh_pe_get(void *data, void *flag); > void *eeh_pe_traverse(struct eeh_pe *root, > eeh_traverse_func fn, void *flag) > { >@@ -179,6 +180,8 @@ void *eeh_pe_traverse(struct eeh_pe *root, > void *ret; > > for (pe = root; pe; pe = eeh_pe_next(pe, root)) { >+ if (eeh_pe_passed(pe) && (fn != __eeh_pe_get)) >+ continue; The code change here seems ugly. The "flag" can be extended to carry the information to skip pass-through PEs or not. So the function calling eeh_pe_traverse() decides to skip pass-through PEs or not. > ret = fn(pe, flag); > if (ret) return ret; > } >@@ -210,6 +213,8 @@ void *eeh_pe_dev_traverse(struct eeh_pe *root, > > /* Traverse root PE */ > for (pe = root; pe; pe = eeh_pe_next(pe, root)) { >+ if (eeh_pe_passed(pe)) >+ continue; > eeh_pe_for_each_dev(pe, edev, tmp) { > ret = fn(edev, flag); > if (ret) >-- >2.5.0 > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] powerpc/eeh: Avoid to handle EEH on a passed Child PE 2015-09-21 11:49 ` Gavin Shan @ 2015-09-22 4:43 ` Wei Yang 2015-09-22 23:07 ` Gavin Shan 0 siblings, 1 reply; 5+ messages in thread From: Wei Yang @ 2015-09-22 4:43 UTC (permalink / raw) To: Gavin Shan; +Cc: Wei Yang, linuxppc-dev On Mon, Sep 21, 2015 at 09:49:45PM +1000, Gavin Shan wrote: >On Mon, Sep 21, 2015 at 05:29:48PM +0800, Wei Yang wrote: >>Current EEH infrastructure would avoid to handle EEH when a PE is passed to >>guest, while if this PE is a Child PE of the one hit EEH, host would handle >>this. By doing so, this would leads to guest hang. The correct way is >>avoid to handle it on host and let guest to recover. >> >>This patch avoids to handle EEH on a passed Child PE. >> > >Ok. It's fixing the problem the guest, which owns a VF, when its PF hitting >EEH error, right? If so, I'm not sure if you really tested this code. Does >it work for you? Yes, I inject error on Parent Bus PE. > >When the parent PE (PF) is stopped for EEH recovery, it sounds impossible >that the child PE can't be affected and just escape from the error. The >question is how the guest can continue to work after the EEH recovery on >parent PE? What I see is the PF is covering and VF in guest is recovering. > >>Signed-off-by: Wei Yang <weiyang@linux.vnet.ibm.com> >>--- >> arch/powerpc/kernel/eeh_pe.c | 5 +++++ >> 1 file changed, 5 insertions(+) >> >>diff --git a/arch/powerpc/kernel/eeh_pe.c b/arch/powerpc/kernel/eeh_pe.c >>index 5cde950..c6d0e9f 100644 >>--- a/arch/powerpc/kernel/eeh_pe.c >>+++ b/arch/powerpc/kernel/eeh_pe.c >>@@ -172,6 +172,7 @@ static struct eeh_pe *eeh_pe_next(struct eeh_pe *pe, >> * callback returns something other than NULL, or no more PEs >> * to be traversed. >> */ >>+static void *__eeh_pe_get(void *data, void *flag); >> void *eeh_pe_traverse(struct eeh_pe *root, >> eeh_traverse_func fn, void *flag) >> { >>@@ -179,6 +180,8 @@ void *eeh_pe_traverse(struct eeh_pe *root, >> void *ret; >> >> for (pe = root; pe; pe = eeh_pe_next(pe, root)) { >>+ if (eeh_pe_passed(pe) && (fn != __eeh_pe_get)) >>+ continue; > >The code change here seems ugly. > >The "flag" can be extended to carry the information to skip pass-through >PEs or not. So the function calling eeh_pe_traverse() decides to skip >pass-through PEs or not. I don't get the point, which "flag" you mean? Add a flag in eeh_pe? > >> ret = fn(pe, flag); >> if (ret) return ret; >> } >>@@ -210,6 +213,8 @@ void *eeh_pe_dev_traverse(struct eeh_pe *root, > >> >> /* Traverse root PE */ >> for (pe = root; pe; pe = eeh_pe_next(pe, root)) { >>+ if (eeh_pe_passed(pe)) >>+ continue; >> eeh_pe_for_each_dev(pe, edev, tmp) { >> ret = fn(edev, flag); >> if (ret) >>-- >>2.5.0 >> -- Richard Yang Help you, Help me ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] powerpc/eeh: Avoid to handle EEH on a passed Child PE 2015-09-22 4:43 ` Wei Yang @ 2015-09-22 23:07 ` Gavin Shan 2015-09-25 8:19 ` Wei Yang 0 siblings, 1 reply; 5+ messages in thread From: Gavin Shan @ 2015-09-22 23:07 UTC (permalink / raw) To: Wei Yang; +Cc: Gavin Shan, linuxppc-dev On Tue, Sep 22, 2015 at 12:43:03PM +0800, Wei Yang wrote: >On Mon, Sep 21, 2015 at 09:49:45PM +1000, Gavin Shan wrote: >>On Mon, Sep 21, 2015 at 05:29:48PM +0800, Wei Yang wrote: >>>Current EEH infrastructure would avoid to handle EEH when a PE is passed to >>>guest, while if this PE is a Child PE of the one hit EEH, host would handle >>>this. By doing so, this would leads to guest hang. The correct way is >>>avoid to handle it on host and let guest to recover. >>> >>>This patch avoids to handle EEH on a passed Child PE. >>> >> >>Ok. It's fixing the problem the guest, which owns a VF, when its PF hitting >>EEH error, right? If so, I'm not sure if you really tested this code. Does >>it work for you? > >Yes, I inject error on Parent Bus PE. > >> >>When the parent PE (PF) is stopped for EEH recovery, it sounds impossible >>that the child PE can't be affected and just escape from the error. The >>question is how the guest can continue to work after the EEH recovery on >>parent PE? > >What I see is the PF is covering and VF in guest is recovering. > What do you mean by "covering"? Which PE's error is detected first in your testing? There is potentially race here: when the VF PE's error is detected first and guest tries to recover it. After the recovery happened on guest side, the host detects the PF PE's error and tries to recover it. During the recovery, the VF PE is total unusable but guest doesn't know that and operate like usual. I'm not sure what kinds of problems it can incur, but it would incur issues. On the other hand, if PF PE's error is detected on host first, and then the guest detects the error on VF PE. After that, the host and guest try to do recovery at same time. Host issues PE reset to PF PE, which isn't finished yet. Then guest issues PE reset to VF PE, which will cause another EEH error. So I'm not sure if had this patch fully tested. If so, it seems the result is just achieved by luck, I guess. >> >>>Signed-off-by: Wei Yang <weiyang@linux.vnet.ibm.com> >>>--- >>> arch/powerpc/kernel/eeh_pe.c | 5 +++++ >>> 1 file changed, 5 insertions(+) >>> >>>diff --git a/arch/powerpc/kernel/eeh_pe.c b/arch/powerpc/kernel/eeh_pe.c >>>index 5cde950..c6d0e9f 100644 >>>--- a/arch/powerpc/kernel/eeh_pe.c >>>+++ b/arch/powerpc/kernel/eeh_pe.c >>>@@ -172,6 +172,7 @@ static struct eeh_pe *eeh_pe_next(struct eeh_pe *pe, >>> * callback returns something other than NULL, or no more PEs >>> * to be traversed. >>> */ >>>+static void *__eeh_pe_get(void *data, void *flag); >>> void *eeh_pe_traverse(struct eeh_pe *root, >>> eeh_traverse_func fn, void *flag) >>> { >>>@@ -179,6 +180,8 @@ void *eeh_pe_traverse(struct eeh_pe *root, >>> void *ret; >>> >>> for (pe = root; pe; pe = eeh_pe_next(pe, root)) { >>>+ if (eeh_pe_passed(pe) && (fn != __eeh_pe_get)) >>>+ continue; >> >>The code change here seems ugly. >> >>The "flag" can be extended to carry the information to skip pass-through >>PEs or not. So the function calling eeh_pe_traverse() decides to skip >>pass-through PEs or not. > >I don't get the point, which "flag" you mean? Add a flag in eeh_pe? > >>> void *eeh_pe_traverse(struct eeh_pe *root, >>> eeh_traverse_func fn, void *flag) ^^^^^^^^^^ This one The code needn't to be changed in a hurry though. I don't think it's right way to fix the issue as discussed as above. >> >>> ret = fn(pe, flag); >>> if (ret) return ret; >>> } >>>@@ -210,6 +213,8 @@ void *eeh_pe_dev_traverse(struct eeh_pe *root, >> >>> >>> /* Traverse root PE */ >>> for (pe = root; pe; pe = eeh_pe_next(pe, root)) { >>>+ if (eeh_pe_passed(pe)) >>>+ continue; >>> eeh_pe_for_each_dev(pe, edev, tmp) { >>> ret = fn(edev, flag); >>> if (ret) >>>-- >>>2.5.0 >>> > >-- >Richard Yang >Help you, Help me ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] powerpc/eeh: Avoid to handle EEH on a passed Child PE 2015-09-22 23:07 ` Gavin Shan @ 2015-09-25 8:19 ` Wei Yang 0 siblings, 0 replies; 5+ messages in thread From: Wei Yang @ 2015-09-25 8:19 UTC (permalink / raw) To: Gavin Shan; +Cc: Wei Yang, linuxppc-dev On Wed, Sep 23, 2015 at 09:07:41AM +1000, Gavin Shan wrote: >On Tue, Sep 22, 2015 at 12:43:03PM +0800, Wei Yang wrote: >>On Mon, Sep 21, 2015 at 09:49:45PM +1000, Gavin Shan wrote: >>>On Mon, Sep 21, 2015 at 05:29:48PM +0800, Wei Yang wrote: >>>>Current EEH infrastructure would avoid to handle EEH when a PE is passed to >>>>guest, while if this PE is a Child PE of the one hit EEH, host would handle >>>>this. By doing so, this would leads to guest hang. The correct way is >>>>avoid to handle it on host and let guest to recover. >>>> >>>>This patch avoids to handle EEH on a passed Child PE. >>>> >>> >>>Ok. It's fixing the problem the guest, which owns a VF, when its PF hitting >>>EEH error, right? If so, I'm not sure if you really tested this code. Does >>>it work for you? >> >>Yes, I inject error on Parent Bus PE. >> >>> >>>When the parent PE (PF) is stopped for EEH recovery, it sounds impossible >>>that the child PE can't be affected and just escape from the error. The >>>question is how the guest can continue to work after the EEH recovery on >>>parent PE? >> >>What I see is the PF is covering and VF in guest is recovering. >> > >What do you mean by "covering"? Which PE's error is detected first in your >testing? There is potentially race here: when the VF PE's error is detected >first and guest tries to recover it. After the recovery happened on guest >side, the host detects the PF PE's error and tries to recover it. During >the recovery, the VF PE is total unusable but guest doesn't know that and >operate like usual. I'm not sure what kinds of problems it can incur, but >it would incur issues. > >On the other hand, if PF PE's error is detected on host first, and then the >guest detects the error on VF PE. After that, the host and guest try to do >recovery at same time. Host issues PE reset to PF PE, which isn't finished >yet. Then guest issues PE reset to VF PE, which will cause another EEH error. > >So I'm not sure if had this patch fully tested. If so, it seems the result is >just achieved by luck, I guess. > It looks really has some race condition. So the expected order should be PF's PE recovered then VF's PE recover in guest, right? >>> >>>>Signed-off-by: Wei Yang <weiyang@linux.vnet.ibm.com> >>>>--- >>>> arch/powerpc/kernel/eeh_pe.c | 5 +++++ >>>> 1 file changed, 5 insertions(+) >>>> >>>>diff --git a/arch/powerpc/kernel/eeh_pe.c b/arch/powerpc/kernel/eeh_pe.c >>>>index 5cde950..c6d0e9f 100644 >>>>--- a/arch/powerpc/kernel/eeh_pe.c >>>>+++ b/arch/powerpc/kernel/eeh_pe.c >>>>@@ -172,6 +172,7 @@ static struct eeh_pe *eeh_pe_next(struct eeh_pe *pe, >>>> * callback returns something other than NULL, or no more PEs >>>> * to be traversed. >>>> */ >>>>+static void *__eeh_pe_get(void *data, void *flag); >>>> void *eeh_pe_traverse(struct eeh_pe *root, >>>> eeh_traverse_func fn, void *flag) >>>> { >>>>@@ -179,6 +180,8 @@ void *eeh_pe_traverse(struct eeh_pe *root, >>>> void *ret; >>>> >>>> for (pe = root; pe; pe = eeh_pe_next(pe, root)) { >>>>+ if (eeh_pe_passed(pe) && (fn != __eeh_pe_get)) >>>>+ continue; >>> >>>The code change here seems ugly. >>> >>>The "flag" can be extended to carry the information to skip pass-through >>>PEs or not. So the function calling eeh_pe_traverse() decides to skip >>>pass-through PEs or not. >> >>I don't get the point, which "flag" you mean? Add a flag in eeh_pe? >> > >>>> void *eeh_pe_traverse(struct eeh_pe *root, >>>> eeh_traverse_func fn, void *flag) > ^^^^^^^^^^ > This one > >The code needn't to be changed in a hurry though. I don't think it's right >way to fix the issue as discussed as above. > >>> >>>> ret = fn(pe, flag); >>>> if (ret) return ret; >>>> } >>>>@@ -210,6 +213,8 @@ void *eeh_pe_dev_traverse(struct eeh_pe *root, >>> >>>> >>>> /* Traverse root PE */ >>>> for (pe = root; pe; pe = eeh_pe_next(pe, root)) { >>>>+ if (eeh_pe_passed(pe)) >>>>+ continue; >>>> eeh_pe_for_each_dev(pe, edev, tmp) { >>>> ret = fn(edev, flag); >>>> if (ret) >>>>-- >>>>2.5.0 >>>> >> >>-- >>Richard Yang >>Help you, Help me -- Richard Yang Help you, Help me ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2015-09-25 8:20 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2015-09-21 9:29 [PATCH] powerpc/eeh: Avoid to handle EEH on a passed Child PE Wei Yang 2015-09-21 11:49 ` Gavin Shan 2015-09-22 4:43 ` Wei Yang 2015-09-22 23:07 ` Gavin Shan 2015-09-25 8:19 ` Wei Yang
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).