[PATCH 1/2] powerpc/papr_scm: Update drc_pmem_unbind() to use H_SCM_UNBIND_ALL

[Oliver O'Halloran]

On Tue, Jun 25, 2019 at 1:03 AM Vaibhav Jain <vaibhav at linux.ibm.com> wrote:
>
> The new hcall named H_SCM_UNBIND_ALL has been introduce that can
> unbind all the memory drc memory-blocks assigned to an lpar. This is
> more efficient than using H_SCM_UNBIND_MEM as currently we don't
> support partial unbind of drc memory-blocks.
>
> Hence this patch proposes following changes to drc_pmem_unbind():
>
>     * Update drc_pmem_unbind() to replace hcall H_SCM_UNBIND_MEM to
>       H_SCM_UNBIND_ALL.
>
>     * Update drc_pmem_unbind() to handles cases when PHYP asks the guest
>       kernel to wait for specific amount of time before retrying the
>       hcall via the 'LONG_BUSY' return value. In case it takes more
>       than 1 second to unbind the memory log a warning.

Have you benchmarked how long a typical bind operation takes for very
large (1+TB) volumes? I remember it taking a while for the driver to
finish probing and I'd rather we didn't add spurious warnings. That
might have been due to the time taken to online memory rather than the
bind though.

>     * Ensure appropriate error code is returned back from the function
>       in case of an error.
>
> Signed-off-by: Vaibhav Jain <vaibhav at linux.ibm.com>
> ---
>  arch/powerpc/include/asm/hvcall.h         |  2 +-
>  arch/powerpc/platforms/pseries/papr_scm.c | 37 ++++++++++++++++++++---
>  2 files changed, 33 insertions(+), 6 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/hvcall.h b/arch/powerpc/include/asm/hvcall.h
> index 463c63a9fcf1..bb56fa0f976c 100644
> --- a/arch/powerpc/include/asm/hvcall.h
> +++ b/arch/powerpc/include/asm/hvcall.h
> @@ -302,7 +302,7 @@
>  #define H_SCM_UNBIND_MEM        0x3F0
>  #define H_SCM_QUERY_BLOCK_MEM_BINDING 0x3F4
>  #define H_SCM_QUERY_LOGICAL_MEM_BINDING 0x3F8
> -#define H_SCM_MEM_QUERY                0x3FC
> +#define H_SCM_UNBIND_ALL        0x3FC
>  #define H_SCM_BLOCK_CLEAR       0x400
>  #define MAX_HCALL_OPCODE       H_SCM_BLOCK_CLEAR

We should probably update all these to match the latest spec. Can you
do that in a separate patch?

> diff --git a/arch/powerpc/platforms/pseries/papr_scm.c b/arch/powerpc/platforms/pseries/papr_scm.c
> index 96c53b23e58f..d790e4e4ffb3 100644
> --- a/arch/powerpc/platforms/pseries/papr_scm.c
> +++ b/arch/powerpc/platforms/pseries/papr_scm.c
> @@ -11,11 +11,16 @@
>  #include <linux/sched.h>
>  #include <linux/libnvdimm.h>
>  #include <linux/platform_device.h>
> +#include <linux/delay.h>
>
>  #include <asm/plpar_wrappers.h>
>
>  #define BIND_ANY_ADDR (~0ul)
>
> +/* Scope args for H_SCM_UNBIND_ALL */
> +#define H_UNBIND_SCOPE_ALL (0x1)
> +#define H_UNBIND_SCOPE_DRC (0x2)
> +
>  #define PAPR_SCM_DIMM_CMD_MASK \
>         ((1ul << ND_CMD_GET_CONFIG_SIZE) | \
>          (1ul << ND_CMD_GET_CONFIG_DATA) | \
> @@ -78,21 +83,43 @@ static int drc_pmem_unbind(struct papr_scm_priv *p)
>  {
>         unsigned long ret[PLPAR_HCALL_BUFSIZE];
>         uint64_t rc, token;
> +       unsigned long starttime;
>
>         token = 0;
>
> -       /* NB: unbind has the same retry requirements mentioned above */
> +       dev_dbg(&p->pdev->dev, "unbind drc %x\n", p->drc_index);
> +
> +       /* NB: unbind_all has the same retry requirements as drc_pmem_bind() */
> +       starttime = HZ;
>         do {
> -               rc = plpar_hcall(H_SCM_UNBIND_MEM, ret, p->drc_index,
> -                               p->bound_addr, p->blocks, token);
> +               /* If this is taking too much time then log warning */
> +               if (jiffies_to_msecs(HZ - starttime) > 1000) {
> +                       dev_warn(&p->pdev->dev,
> +                                "unbind taking > 1s to complete\n");
> +                       starttime = HZ;
> +               }
> +
> +               /* Unbind of all SCM resources associated with drcIndex */
> +               rc = plpar_hcall(H_SCM_UNBIND_ALL, ret, H_UNBIND_SCOPE_DRC,
> +                                p->drc_index, token);
>                 token = ret[0];
> -               cond_resched();
> +
> +               /* Check if we are stalled for some time */
> +               if (H_IS_LONG_BUSY(rc)) {
> +                       msleep(get_longbusy_msecs(rc));
> +                       rc = H_BUSY;

> +                       token = 0;

The hypervisor should be returning a continue token if the return code
is H_BUSY or any of the H_LONG_BUSY codes. The spec says that if we
get a busy return value then we must use the continue token for the
next unbind, so why are you zeroing it here? If this is required to
make it work then it's a bug in the hypervisor.

> +               } else if (rc == H_BUSY) {
> +                       cond_resched();
> +               }
> +
>         } while (rc == H_BUSY);
>
>         if (rc)
>                 dev_err(&p->pdev->dev, "unbind error: %lld\n", rc);

A dev_dbg() for the unbind time might be nice.

>
> -       return !!rc;
> +       return rc == H_SUCCESS ? 0 : -ENXIO;
>  }
>
>  static int papr_scm_meta_get(struct papr_scm_priv *p,
> --
> 2.21.0
>