| Message ID | 20210211225246.17315-2-danielhb413@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | CPU unplug timeout/LMB unplug cleanup in DRC reconfiguration | expand |
On Thu, 11 Feb 2021 19:52:40 -0300 Daniel Henrique Barboza <danielhb413@gmail.com> wrote: > drc_isolate_logical() is used to move the DRC from the "Configured" to the > "Available" state, erroring out if the DRC is in the unexpected "Unisolate" > state and doing nothing (with RTAS_OUT_SUCCESS) if the DRC is already in > "Available" or in "Unusable" state. > > When moving from "Configured" to "Available", the DRC is moved to the > LOGICAL_AVAILABLE state, a drc->unplug_requested check is done and, if true, > spapr_drc_detach() is called. > > What spapr_drc_detach() does then is: > > - set drc->unplug_requested to true. In fact, this is the only place where > unplug_request is set to true; > - does nothing else if drc->state != drck->empty_state. If the DRC state > is equal to drck->empty_state, spapr_drc_release() is called. For logical > DRCs, drck->empty_state = LOGICAL_UNUSABLE. > > In short, calling spapr_drc_detach() in drc_isolate_logical() does nothing. It'll > set unplug_request to true again ('again' since it was already true - otherwise the > function wouldn't be called), and will return without calling spapr_drc_release() > because the DRC is not in LOGICAL_UNUSABLE, since drc_isolate_logical() just > moved it to LOGICAL_AVAILABLE. The only place where the logical DRC is released > is when called from drc_set_unusable(), when it is moved to the "Unusable" state. > As it should, according to PAPR. > > Even though calling spapr_drc_detach() in drc_isolate_logical() is benign, removing > it will avoid further thought about the matter. So let's go ahead and do that. > Good catch. This path looks useless for logical DRCs indeed. > As a note, this logic was introduced in commit bbf5c878ab76. Since then, the DRC > handling code was refactored and enhanced, and PAPR itself went through some > changes in the DRC area as well. It is expected that some assumptions we had back > then are now deprecated. > As specified in [1]: Please do not use lines that are longer than 76 characters in your commit message (so that the text still shows up nicely with "git show" in a 80-columns terminal window). [1] https://wiki.qemu.org/Contribute/SubmitAPatch#Write_a_meaningful_commit_message > Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> > --- > hw/ppc/spapr_drc.c | 13 ------------- > 1 file changed, 13 deletions(-) > > diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c > index 8571d5bafe..84bd3c881f 100644 > --- a/hw/ppc/spapr_drc.c > +++ b/hw/ppc/spapr_drc.c > @@ -132,19 +132,6 @@ static uint32_t drc_isolate_logical(SpaprDrc *drc) > > drc->state = SPAPR_DRC_STATE_LOGICAL_AVAILABLE; > > - /* if we're awaiting release, but still in an unconfigured state, > - * it's likely the guest is still in the process of configuring > - * the device and is transitioning the devices to an ISOLATED > - * state as a part of that process. so we only complete the > - * removal when this transition happens for a device in a > - * configured state, as suggested by the state diagram from PAPR+ > - * 2.7, 13.4 > - */ > - if (drc->unplug_requested) { > - uint32_t drc_index = spapr_drc_index(drc); > - trace_spapr_drc_set_isolation_state_finalizing(drc_index); I was expecting a change in hw/ppc/trace-events to ditch this trace, but it is still called by drc_isolate_physical(), so we're good. Reviewed-by: Greg Kurz <groug@kaod.org> > - spapr_drc_detach(drc); > - } > return RTAS_OUT_SUCCESS; > } >
On Mon, Feb 15, 2021 at 11:40:06AM +0100, Greg Kurz wrote: > On Thu, 11 Feb 2021 19:52:40 -0300 > Daniel Henrique Barboza <danielhb413@gmail.com> wrote: > > > drc_isolate_logical() is used to move the DRC from the "Configured" to the > > "Available" state, erroring out if the DRC is in the unexpected "Unisolate" > > state and doing nothing (with RTAS_OUT_SUCCESS) if the DRC is already in > > "Available" or in "Unusable" state. > > > > When moving from "Configured" to "Available", the DRC is moved to the > > LOGICAL_AVAILABLE state, a drc->unplug_requested check is done and, if true, > > spapr_drc_detach() is called. > > > > What spapr_drc_detach() does then is: > > > > - set drc->unplug_requested to true. In fact, this is the only place where > > unplug_request is set to true; > > - does nothing else if drc->state != drck->empty_state. If the DRC state > > is equal to drck->empty_state, spapr_drc_release() is called. For logical > > DRCs, drck->empty_state = LOGICAL_UNUSABLE. > > > > In short, calling spapr_drc_detach() in drc_isolate_logical() does nothing. It'll > > set unplug_request to true again ('again' since it was already true - otherwise the > > function wouldn't be called), and will return without calling spapr_drc_release() > > because the DRC is not in LOGICAL_UNUSABLE, since drc_isolate_logical() just > > moved it to LOGICAL_AVAILABLE. The only place where the logical DRC is released > > is when called from drc_set_unusable(), when it is moved to the "Unusable" state. > > As it should, according to PAPR. > > > > Even though calling spapr_drc_detach() in drc_isolate_logical() is benign, removing > > it will avoid further thought about the matter. So let's go ahead and do that. > > > > Good catch. This path looks useless for logical DRCs indeed. > > > As a note, this logic was introduced in commit bbf5c878ab76. Since then, the DRC > > handling code was refactored and enhanced, and PAPR itself went through some > > changes in the DRC area as well. It is expected that some assumptions we had back > > then are now deprecated. > > > > As specified in [1]: > > Please do not use lines that are longer than 76 characters in your > commit message (so that the text still shows up nicely with "git show" > in a 80-columns terminal window). > > [1] https://wiki.qemu.org/Contribute/SubmitAPatch#Write_a_meaningful_commit_message I've applied this patch, but re-wrapped the commit message. > > Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> > > --- > > hw/ppc/spapr_drc.c | 13 ------------- > > 1 file changed, 13 deletions(-) > > > > diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c > > index 8571d5bafe..84bd3c881f 100644 > > --- a/hw/ppc/spapr_drc.c > > +++ b/hw/ppc/spapr_drc.c > > @@ -132,19 +132,6 @@ static uint32_t drc_isolate_logical(SpaprDrc *drc) > > > > drc->state = SPAPR_DRC_STATE_LOGICAL_AVAILABLE; > > > > - /* if we're awaiting release, but still in an unconfigured state, > > - * it's likely the guest is still in the process of configuring > > - * the device and is transitioning the devices to an ISOLATED > > - * state as a part of that process. so we only complete the > > - * removal when this transition happens for a device in a > > - * configured state, as suggested by the state diagram from PAPR+ > > - * 2.7, 13.4 > > - */ > > - if (drc->unplug_requested) { > > - uint32_t drc_index = spapr_drc_index(drc); > > - trace_spapr_drc_set_isolation_state_finalizing(drc_index); > > I was expecting a change in hw/ppc/trace-events to ditch this trace, > but it is still called by drc_isolate_physical(), so we're good. > > Reviewed-by: Greg Kurz <groug@kaod.org> > > > - spapr_drc_detach(drc); > > - } > > return RTAS_OUT_SUCCESS; > > } > > >
diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c index 8571d5bafe..84bd3c881f 100644 --- a/hw/ppc/spapr_drc.c +++ b/hw/ppc/spapr_drc.c @@ -132,19 +132,6 @@ static uint32_t drc_isolate_logical(SpaprDrc *drc) drc->state = SPAPR_DRC_STATE_LOGICAL_AVAILABLE; - /* if we're awaiting release, but still in an unconfigured state, - * it's likely the guest is still in the process of configuring - * the device and is transitioning the devices to an ISOLATED - * state as a part of that process. so we only complete the - * removal when this transition happens for a device in a - * configured state, as suggested by the state diagram from PAPR+ - * 2.7, 13.4 - */ - if (drc->unplug_requested) { - uint32_t drc_index = spapr_drc_index(drc); - trace_spapr_drc_set_isolation_state_finalizing(drc_index); - spapr_drc_detach(drc); - } return RTAS_OUT_SUCCESS; }
drc_isolate_logical() is used to move the DRC from the "Configured" to the "Available" state, erroring out if the DRC is in the unexpected "Unisolate" state and doing nothing (with RTAS_OUT_SUCCESS) if the DRC is already in "Available" or in "Unusable" state. When moving from "Configured" to "Available", the DRC is moved to the LOGICAL_AVAILABLE state, a drc->unplug_requested check is done and, if true, spapr_drc_detach() is called. What spapr_drc_detach() does then is: - set drc->unplug_requested to true. In fact, this is the only place where unplug_request is set to true; - does nothing else if drc->state != drck->empty_state. If the DRC state is equal to drck->empty_state, spapr_drc_release() is called. For logical DRCs, drck->empty_state = LOGICAL_UNUSABLE. In short, calling spapr_drc_detach() in drc_isolate_logical() does nothing. It'll set unplug_request to true again ('again' since it was already true - otherwise the function wouldn't be called), and will return without calling spapr_drc_release() because the DRC is not in LOGICAL_UNUSABLE, since drc_isolate_logical() just moved it to LOGICAL_AVAILABLE. The only place where the logical DRC is released is when called from drc_set_unusable(), when it is moved to the "Unusable" state. As it should, according to PAPR. Even though calling spapr_drc_detach() in drc_isolate_logical() is benign, removing it will avoid further thought about the matter. So let's go ahead and do that. As a note, this logic was introduced in commit bbf5c878ab76. Since then, the DRC handling code was refactored and enhanced, and PAPR itself went through some changes in the DRC area as well. It is expected that some assumptions we had back then are now deprecated. Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> --- hw/ppc/spapr_drc.c | 13 ------------- 1 file changed, 13 deletions(-)