Re: [PATCH 2/5] xen/domain: Improve pirq handling

[Jan Beulich <jbeulich@xxxxxxxx>]

On 11.11.2021 18:57, Andrew Cooper wrote:
> free_pirq_struct() has no external users, so shouldn't be exposed.

This has been the case from its very introduction. Which iirc was done
that way because its alloc counterpart is non-static. Not an objection,
just an observation.

>  Making it
> static necessitates moving the function as domain_destroy() uses it.
> 
> Rework pirq_get_info() to have easier-to-follow logic.

That's a matter of taste; I for one would prefer the original form with
just a single return statement. I'm (obviously) not going to nack this,
but I'm not sure yet whether I'm willing to (eventually) ack it.

>  The one functional
> change is to the insertion failure path; we should not be using a full
> call_rcu() chain to free an otherwise local structure we failed to insert into
> the radix tree to begin with.

This makes an assumption on the radix tree implementation, in that failure
there may not occur after publication of the pointer. This perhaps is not
a requirement that would easily get violated considering the present code
structure, but I'm still not sure we want to have such hidden dependencies.
At the very least I seem to vaguely recall that at the time of
introduction it wasn't just an oversight to use the RCU approach there as
well.

> --- a/xen/common/domain.c
> +++ b/xen/common/domain.c
> @@ -365,6 +365,39 @@ static int __init parse_extra_guest_irqs(const char *s)
>  }
>  custom_param("extra_guest_irqs", parse_extra_guest_irqs);
>  
> +static void _free_pirq_struct(struct rcu_head *head)
> +{
> +    xfree(container_of(head, struct pirq, rcu_head));
> +}
> +
> +static void free_pirq_struct(void *ptr)
> +{
> +    struct pirq *pirq = ptr;
> +
> +    call_rcu(&pirq->rcu_head, _free_pirq_struct);
> +}
> +
> +struct pirq *pirq_get_info(struct domain *d, int pirq)
> +{
> +    struct pirq *info = pirq_info(d, pirq);
> +
> +    if ( likely(info) )
> +        return info;
> +
> +    info = alloc_pirq_struct(d);
> +    if ( unlikely(!info) )
> +        return NULL;

Are the unlikely() here and ...

> +    info->pirq = pirq;
> +    if ( likely(radix_tree_insert(&d->pirq_tree, pirq, info) == 0) )
> +        return info; /* Success. */

... the likely() here really warranted? Iirc you're generally advocating
for avoiding their use unless strongly indicated, and if I'm not mistaken
the compiler's heuristics result in such NULL / 0 checks to get assumed
to be unlikely / likely anyway.

Jan