Re: [PATCH 6/6] xive: Don't use CPU_FOREACH() to perform CAM line matching

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 5169 bytes --]

On Wed, Oct 23, 2019 at 04:52:27PM +0200, Greg Kurz wrote:
> Now that the TCTX objects are children of the XIVE router, stop
> using CPU_FOREACH() when looking for a matching VCPU target.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>
> ---
>  hw/intc/xive.c |  100 +++++++++++++++++++++++++++++++++++---------------------
>  1 file changed, 62 insertions(+), 38 deletions(-)
> 
> diff --git a/hw/intc/xive.c b/hw/intc/xive.c
> index 40ce43152456..ec5e7d0ee39a 100644
> --- a/hw/intc/xive.c
> +++ b/hw/intc/xive.c
> @@ -1403,55 +1403,79 @@ typedef struct XiveTCTXMatch {
>      uint8_t ring;
>  } XiveTCTXMatch;
>  
> -static bool xive_presenter_match(XiveRouter *xrtr, uint8_t format,
> -                                 uint8_t nvt_blk, uint32_t nvt_idx,
> -                                 bool cam_ignore, uint8_t priority,
> -                                 uint32_t logic_serv, XiveTCTXMatch *match)
> +typedef struct XivePresenterMatch {
> +    uint8_t format;
> +    uint8_t nvt_blk;
> +    uint32_t nvt_idx;
> +    bool cam_ignore;
> +    uint8_t priority;
> +    uint32_t logic_serv;
> +    XiveTCTXMatch *match;
> +    int count;
> +} XivePresenterMatch;
> +
> +static int do_xive_presenter_match(Object *child, void *opaque)
>  {
> -    CPUState *cs;
> +    XiveTCTX *tctx = XIVE_TCTX(child);
> +    XivePresenterMatch *xpm = opaque;
> +    int ring;
>  
>      /*
>       * TODO (PowerNV): handle chip_id overwrite of block field for
>       * hardwired CAM compares
>       */
>  
> -    CPU_FOREACH(cs) {
> -        XiveTCTX *tctx = xive_router_get_tctx(xrtr, cs);
> -        int ring;
> +    /*
> +     * HW checks that the CPU is enabled in the Physical Thread
> +     * Enable Register (PTER).
> +     */
>  
> -        /*
> -         * Skip partially initialized vCPUs. This can happen when
> -         * vCPUs are hotplugged.
> -         */
> -        if (!tctx) {
> -            continue;
> +    /*
> +     * Check the thread context CAM lines and record matches. We
> +     * will handle CPU exception delivery later
> +     */
> +    ring = xive_presenter_tctx_match(tctx, xpm->format, xpm->nvt_blk,
> +                                     xpm->nvt_idx, xpm->cam_ignore,
> +                                     xpm->logic_serv);
> +
> +    /*
> +     * Save the context and follow on to catch duplicates, that we
> +     * don't support yet.
> +     */
> +    if (ring != -1) {
> +        if (xpm->match->tctx) {
> +            qemu_log_mask(LOG_GUEST_ERROR, "XIVE: already found a thread "
> +                          "context NVT %x/%x\n", xpm->nvt_blk, xpm->nvt_idx);
> +            return -1;
>          }
>  
> -        /*
> -         * HW checks that the CPU is enabled in the Physical Thread
> -         * Enable Register (PTER).
> -         */
> +        xpm->match->ring = ring;
> +        xpm->match->tctx = tctx;
> +        xpm->count++;
> +    }
>  
> -        /*
> -         * Check the thread context CAM lines and record matches. We
> -         * will handle CPU exception delivery later
> -         */
> -        ring = xive_presenter_tctx_match(tctx, format, nvt_blk, nvt_idx,
> -                                         cam_ignore, logic_serv);
> -        /*
> -         * Save the context and follow on to catch duplicates, that we
> -         * don't support yet.
> -         */
> -        if (ring != -1) {
> -            if (match->tctx) {
> -                qemu_log_mask(LOG_GUEST_ERROR, "XIVE: already found a thread "
> -                              "context NVT %x/%x\n", nvt_blk, nvt_idx);
> -                return false;
> -            }
> -
> -            match->ring = ring;
> -            match->tctx = tctx;
> -        }
> +    return 0;
> +}
> +
> +static bool xive_presenter_match(XiveRouter *xrtr, uint8_t format,
> +                                 uint8_t nvt_blk, uint32_t nvt_idx,
> +                                 bool cam_ignore, uint8_t priority,
> +                                 uint32_t logic_serv, XiveTCTXMatch *match)
> +{
> +    XivePresenterMatch xpm = {
> +        .format     = format,
> +        .nvt_blk    = nvt_blk,
> +        .nvt_idx    = nvt_idx,
> +        .cam_ignore = cam_ignore,
> +        .priority   = priority,
> +        .logic_serv = logic_serv,
> +        .match      = match,
> +        .count      = 0,
> +    };
> +
> +    if (object_child_foreach_type(OBJECT(xrtr), TYPE_XIVE_TCTX,
> +                                  do_xive_presenter_match, &xpm) < 0) {
> +        return false;

Hrm... xive_presenter_match() is potentially a pretty hot path, it's
called on every interrupt delivery - especially since we don't have a
usable KVM irq chip for Boston machines.  I'm concerned that using
something as heavyweight as object_child_foreach() might have a
noticeable performance impact.

>      }
>  
>      if (!match->tctx) {
> 

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]