Message ID | 1460984701-21490-1-git-send-email-clombard@linux.vnet.ibm.com (mailing list archive) |
---|---|
State | Superseded, archived |
Headers | show |
On 18/04/16 23:05, Christophe Lombard wrote: > In the POWERVM environement, the PHYP CoherentAccel component manages environment > the state of the Coherant Accelerator Processor Interface adapter and Coherent > virtualizes CAPI resources, handles CAPP, PSL, PSL Slice errors - and > interrupts - and provides a new set of HCALLs for the OS APIs to utilize > AFUs. > > During the course of operation, a coherent platform function can > encounter errors. Some possible reason for errors are: > • Hardware recoverable and unrecoverable errors > • Transient and over-threshold correctable errors > > PHYP implements its own state model for the coherent platform function. > The current state of this Acclerator Fonction Unit (AFU) is available Accelerator Function Unit > through a hcall. > > In case of low-level troubles (or error injection), The PHYP component the > may reset the card and change the AFU state. The PHYP interface doesn't > provide any way to be notified when that happens. > > The current implementation of the cxl driver, for the POWERVM > environment, follows the general error recovery procedures required to > reset operation of the coherent platform function. The platform firmware > resets and reconfigures hardware when an external action is required - > attach/detach a process, link ok, .... > > The purpose of this patch is to interact with the external driver > (where the AFU is shown) even if no action is required. A kernel thread > is needed to check every x seconds the current state of the AFU to see > if we need to enter an error recovery path. > > Signed-off-by: Christophe Lombard <clombard@linux.vnet.ibm.com> A few minor issues below. > diff --git a/drivers/misc/cxl/guest.c b/drivers/misc/cxl/guest.c > index 8213372..06dfe7f 100644 > --- a/drivers/misc/cxl/guest.c > +++ b/drivers/misc/cxl/guest.c > @@ -19,6 +19,10 @@ > #define CXL_SLOT_RESET_EVENT 2 > #define CXL_RESUME_EVENT 3 > > +#define CXL_KTHREAD "cxl_kthread" > + > +void stop_state_thread(struct cxl_afu *afu); static? [...] > -static int afu_do_recovery(struct cxl_afu *afu) > +static int handle_state_thread(void *data) > { > - int rc; > + struct cxl_afu *afu; > + int rc = 0; It looks like we don't use rc (see also comment below). > > - /* many threads can arrive here, in case of detach_all for example. > - * Only one needs to drive the recovery > - */ > - if (mutex_trylock(&afu->guest->recovery_lock)) { > - rc = afu_update_state(afu); > - mutex_unlock(&afu->guest->recovery_lock); > - return rc; > + pr_devel("in %s\n", __func__); > + > + afu = (struct cxl_afu*)data; CodingStyle: space between cxl_afu and * > + do { > + set_current_state(TASK_INTERRUPTIBLE); > + > + if (afu) { > + afu_update_state(afu); Should we be checking the retval here? > + if (afu->guest->previous_state == H_STATE_PERM_UNAVAILABLE) > + goto out; > + } else > + return -ENODEV; > + schedule_timeout(msecs_to_jiffies(3000)); > + } while(!kthread_should_stop()); CodingStyle: space between while and ( > + > +out: > + afu->guest->kthread_tsk = NULL; > + return rc; > +} > + > +void start_state_thread(struct cxl_afu *afu) static? > +{ > + if (afu->guest->kthread_tsk) > + return; > + > + /* start kernel thread to handle the state of the afu */ > + afu->guest->kthread_tsk = kthread_run(&handle_state_thread, > + (void *)afu, CXL_KTHREAD); > + if (IS_ERR(afu->guest->kthread_tsk)) { > + pr_devel("cannot start state kthread\n"); > + afu->guest->kthread_tsk = NULL; > } > - return 0; > +} > + > +void stop_state_thread(struct cxl_afu *afu) static?
On 19/04/2016 04:40, Andrew Donnellan wrote: > On 18/04/16 23:05, Christophe Lombard wrote: >> In the POWERVM environement, the PHYP CoherentAccel component manages > > environment > >> the state of the Coherant Accelerator Processor Interface adapter and > > Coherent > >> virtualizes CAPI resources, handles CAPP, PSL, PSL Slice errors - and >> interrupts - and provides a new set of HCALLs for the OS APIs to utilize >> AFUs. >> >> During the course of operation, a coherent platform function can >> encounter errors. Some possible reason for errors are: >> • Hardware recoverable and unrecoverable errors >> • Transient and over-threshold correctable errors >> >> PHYP implements its own state model for the coherent platform function. >> The current state of this Acclerator Fonction Unit (AFU) is available > > Accelerator Function Unit > >> through a hcall. >> >> In case of low-level troubles (or error injection), The PHYP component > > the > >> may reset the card and change the AFU state. The PHYP interface doesn't >> provide any way to be notified when that happens. >> >> The current implementation of the cxl driver, for the POWERVM >> environment, follows the general error recovery procedures required to >> reset operation of the coherent platform function. The platform firmware >> resets and reconfigures hardware when an external action is required - >> attach/detach a process, link ok, .... >> >> The purpose of this patch is to interact with the external driver >> (where the AFU is shown) even if no action is required. A kernel thread >> is needed to check every x seconds the current state of the AFU to see >> if we need to enter an error recovery path. >> >> Signed-off-by: Christophe Lombard <clombard@linux.vnet.ibm.com> > > A few minor issues below. > >> diff --git a/drivers/misc/cxl/guest.c b/drivers/misc/cxl/guest.c >> index 8213372..06dfe7f 100644 >> --- a/drivers/misc/cxl/guest.c >> +++ b/drivers/misc/cxl/guest.c >> @@ -19,6 +19,10 @@ >> #define CXL_SLOT_RESET_EVENT 2 >> #define CXL_RESUME_EVENT 3 >> >> +#define CXL_KTHREAD "cxl_kthread" >> + >> +void stop_state_thread(struct cxl_afu *afu); > > static? > > [...] > >> -static int afu_do_recovery(struct cxl_afu *afu) >> +static int handle_state_thread(void *data) >> { >> - int rc; >> + struct cxl_afu *afu; >> + int rc = 0; > > It looks like we don't use rc (see also comment below). > >> >> - /* many threads can arrive here, in case of detach_all for example. >> - * Only one needs to drive the recovery >> - */ >> - if (mutex_trylock(&afu->guest->recovery_lock)) { >> - rc = afu_update_state(afu); >> - mutex_unlock(&afu->guest->recovery_lock); >> - return rc; >> + pr_devel("in %s\n", __func__); >> + >> + afu = (struct cxl_afu*)data; > > CodingStyle: space between cxl_afu and * > >> + do { >> + set_current_state(TASK_INTERRUPTIBLE); >> + >> + if (afu) { >> + afu_update_state(afu); > > Should we be checking the retval here? Right, We have to check the retval here. Thanks > >> + if (afu->guest->previous_state == H_STATE_PERM_UNAVAILABLE) >> + goto out; >> + } else >> + return -ENODEV; >> + schedule_timeout(msecs_to_jiffies(3000)); >> + } while(!kthread_should_stop()); > > CodingStyle: space between while and ( > >> + >> +out: >> + afu->guest->kthread_tsk = NULL; >> + return rc; >> +} >> + >> +void start_state_thread(struct cxl_afu *afu) > > static? > >> +{ >> + if (afu->guest->kthread_tsk) >> + return; >> + >> + /* start kernel thread to handle the state of the afu */ >> + afu->guest->kthread_tsk = kthread_run(&handle_state_thread, >> + (void *)afu, CXL_KTHREAD); >> + if (IS_ERR(afu->guest->kthread_tsk)) { >> + pr_devel("cannot start state kthread\n"); >> + afu->guest->kthread_tsk = NULL; >> } >> - return 0; >> +} >> + >> +void stop_state_thread(struct cxl_afu *afu) > > static? > Thanks for the review. I will send a patch update.
On Mon, 2016-04-18 at 15:05 +0200, Christophe Lombard wrote: > In the POWERVM environement, the PHYP CoherentAccel component manages PowerVM is correct I think. > the state of the Coherant Accelerator Processor Interface adapter and ^ (CAPI) > virtualizes CAPI resources, handles CAPP, PSL, PSL Slice errors - and > interrupts - and provides a new set of HCALLs for the OS APIs to utilize ^ hcall (as below?) > AFUs. AFUs ? (you define it below) > During the course of operation, a coherent platform function can > encounter errors. Some possible reason for errors are: > • Hardware recoverable and unrecoverable errors > • Transient and over-threshold correctable errors > > PHYP implements its own state model for the coherent platform function. > The current state of this Acclerator Fonction Unit (AFU) is available > through a hcall. > > In case of low-level troubles (or error injection), The PHYP component > may reset the card and change the AFU state. The PHYP interface doesn't > provide any way to be notified when that happens. Ugh. > The current implementation of the cxl driver, for the POWERVM > environment, follows the general error recovery procedures required to What are "the general error recovery procedures" ? > reset operation of the coherent platform function. The platform firmware > resets and reconfigures hardware when an external action is required - > attach/detach a process, link ok, .... Platform firmware does that at our request or by itself? > The purpose of this patch is to interact with the external driver What's an external driver? > (where the AFU is shown) even if no action is required. A kernel thread But no action is required, so why do we need to do anything? > is needed to check every x seconds the current state of the AFU to see > if we need to enter an error recovery path. I don't really understand what this is doing and why we want it. It sounds like we're waking the cpu up every 3 seconds and having it poll the hypervisor, for each AFU? As far as the implementation, I can't see any reason why you need your own kthreads, can't you just use queue_work() ? cheers
diff --git a/drivers/misc/cxl/cxl.h b/drivers/misc/cxl/cxl.h index 38e21cf..a26c210 100644 --- a/drivers/misc/cxl/cxl.h +++ b/drivers/misc/cxl/cxl.h @@ -19,6 +19,7 @@ #include <linux/io.h> #include <linux/pci.h> #include <linux/fs.h> +#include <linux/kthread.h> #include <asm/cputable.h> #include <asm/mmu.h> #include <asm/reg.h> @@ -379,7 +380,7 @@ struct cxl_afu_guest { phys_addr_t p2n_phys; u64 p2n_size; int max_ints; - struct mutex recovery_lock; + struct task_struct *kthread_tsk; int previous_state; }; diff --git a/drivers/misc/cxl/guest.c b/drivers/misc/cxl/guest.c index 8213372..06dfe7f 100644 --- a/drivers/misc/cxl/guest.c +++ b/drivers/misc/cxl/guest.c @@ -19,6 +19,10 @@ #define CXL_SLOT_RESET_EVENT 2 #define CXL_RESUME_EVENT 3 +#define CXL_KTHREAD "cxl_kthread" + +void stop_state_thread(struct cxl_afu *afu); + static void pci_error_handlers(struct cxl_afu *afu, int bus_error_event, pci_channel_state_t state) @@ -178,6 +182,9 @@ static int afu_read_error_state(struct cxl_afu *afu, int *state_out) u64 state; int rc = 0; + if (!afu) + return -EIO; + rc = cxl_h_read_error_state(afu->guest->handle, &state); if (!rc) { WARN_ON(state != H_STATE_NORMAL && @@ -645,6 +652,8 @@ static void guest_release_afu(struct device *dev) idr_destroy(&afu->contexts_idr); + stop_state_thread(afu); + kfree(afu->guest); kfree(afu); } @@ -818,7 +827,6 @@ static int afu_update_state(struct cxl_afu *afu) switch (cur_state) { case H_STATE_NORMAL: afu->guest->previous_state = cur_state; - rc = 1; break; case H_STATE_DISABLE: @@ -834,7 +842,6 @@ static int afu_update_state(struct cxl_afu *afu) pci_error_handlers(afu, CXL_SLOT_RESET_EVENT, pci_channel_io_normal); pci_error_handlers(afu, CXL_RESUME_EVENT, 0); - rc = 1; } afu->guest->previous_state = 0; break; @@ -859,39 +866,61 @@ static int afu_update_state(struct cxl_afu *afu) return rc; } -static int afu_do_recovery(struct cxl_afu *afu) +static int handle_state_thread(void *data) { - int rc; + struct cxl_afu *afu; + int rc = 0; - /* many threads can arrive here, in case of detach_all for example. - * Only one needs to drive the recovery - */ - if (mutex_trylock(&afu->guest->recovery_lock)) { - rc = afu_update_state(afu); - mutex_unlock(&afu->guest->recovery_lock); - return rc; + pr_devel("in %s\n", __func__); + + afu = (struct cxl_afu*)data; + do { + set_current_state(TASK_INTERRUPTIBLE); + + if (afu) { + afu_update_state(afu); + if (afu->guest->previous_state == H_STATE_PERM_UNAVAILABLE) + goto out; + } else + return -ENODEV; + schedule_timeout(msecs_to_jiffies(3000)); + } while(!kthread_should_stop()); + +out: + afu->guest->kthread_tsk = NULL; + return rc; +} + +void start_state_thread(struct cxl_afu *afu) +{ + if (afu->guest->kthread_tsk) + return; + + /* start kernel thread to handle the state of the afu */ + afu->guest->kthread_tsk = kthread_run(&handle_state_thread, + (void *)afu, CXL_KTHREAD); + if (IS_ERR(afu->guest->kthread_tsk)) { + pr_devel("cannot start state kthread\n"); + afu->guest->kthread_tsk = NULL; } - return 0; +} + +void stop_state_thread(struct cxl_afu *afu) +{ + if (afu->guest->kthread_tsk) + kthread_stop(afu->guest->kthread_tsk); } static bool guest_link_ok(struct cxl *cxl, struct cxl_afu *afu) { int state; - if (afu) { - if (afu_read_error_state(afu, &state) || - state != H_STATE_NORMAL) { - if (afu_do_recovery(afu) > 0) { - /* check again in case we've just fixed it */ - if (!afu_read_error_state(afu, &state) && - state == H_STATE_NORMAL) - return true; - } - return false; - } + if (afu && (!afu_read_error_state(afu, &state))) { + if (state == H_STATE_NORMAL) + return true; } - return true; + return false; } static int afu_properties_look_ok(struct cxl_afu *afu) @@ -929,8 +958,6 @@ int cxl_guest_init_afu(struct cxl *adapter, int slice, struct device_node *afu_n return -ENOMEM; } - mutex_init(&afu->guest->recovery_lock); - if ((rc = dev_set_name(&afu->dev, "afu%i.%i", adapter->adapter_num, slice))) @@ -986,6 +1013,8 @@ int cxl_guest_init_afu(struct cxl *adapter, int slice, struct device_node *afu_n afu->enabled = true; + start_state_thread(afu); + if ((rc = cxl_pci_vphb_add(afu))) dev_info(&afu->dev, "Can't register vPHB\n");
In the POWERVM environement, the PHYP CoherentAccel component manages the state of the Coherant Accelerator Processor Interface adapter and virtualizes CAPI resources, handles CAPP, PSL, PSL Slice errors - and interrupts - and provides a new set of HCALLs for the OS APIs to utilize AFUs. During the course of operation, a coherent platform function can encounter errors. Some possible reason for errors are: • Hardware recoverable and unrecoverable errors • Transient and over-threshold correctable errors PHYP implements its own state model for the coherent platform function. The current state of this Acclerator Fonction Unit (AFU) is available through a hcall. In case of low-level troubles (or error injection), The PHYP component may reset the card and change the AFU state. The PHYP interface doesn't provide any way to be notified when that happens. The current implementation of the cxl driver, for the POWERVM environment, follows the general error recovery procedures required to reset operation of the coherent platform function. The platform firmware resets and reconfigures hardware when an external action is required - attach/detach a process, link ok, .... The purpose of this patch is to interact with the external driver (where the AFU is shown) even if no action is required. A kernel thread is needed to check every x seconds the current state of the AFU to see if we need to enter an error recovery path. Signed-off-by: Christophe Lombard <clombard@linux.vnet.ibm.com> --- drivers/misc/cxl/cxl.h | 3 +- drivers/misc/cxl/guest.c | 81 ++++++++++++++++++++++++++++++++---------------- 2 files changed, 57 insertions(+), 27 deletions(-)