Re: [PATCH 2/8] evtchn: refactor low-level event channel port ops

["Jan Beulich" <JBeulich@suse.com>]

>>> On 09.08.13 at 20:08, David Vrabel <david.vrabel@citrix.com> wrote:
> +static void evtchn_2l_set_pending(struct vcpu *v, struct evtchn *evtchn)
> +{
> +    struct domain *d = v->domain;
> +    unsigned port = evtchn->port;
> +
> +    /*
> +     * The following bit operations must happen in strict order.
> +     * NB. On x86, the atomic bit operations also act as memory barriers.
> +     * There is therefore sufficiently strict ordering for this architecture --
> +     * others may require explicit memory barriers.
> +     */
> +
> +    if ( test_and_set_bit(port, &shared_info(d, evtchn_pending)) )
> +        return;
> +
> +    if ( !test_bit        (port, &shared_info(d, evtchn_mask)) &&
> +         !test_and_set_bit(port / BITS_PER_EVTCHN_WORD(d),
> +                           &vcpu_info(v, evtchn_pending_sel)) )

Up to here this is indeed 2-level specific, but the rest of the
function isn't, and would therefore better go back into
generic code.

> +    {
> +        vcpu_mark_events_pending(v);
> +    }
> +
> +    evtchn_check_pollers(d, port);
> +}
>[...]
> +static void evtchn_2l_unmask(struct domain *d, struct evtchn *evtchn)
> +{
> +    struct vcpu *v = d->vcpu[evtchn->notify_vcpu_id];
> +    unsigned port = evtchn->port;
> +
> +    /*
> +     * These operations must happen in strict order. Based on
> +     * evtchn_2l_set_pending() above.
> +     */
> +    if ( test_and_clear_bit(port, &shared_info(d, evtchn_mask)) &&
> +         test_bit          (port, &shared_info(d, evtchn_pending)) &&
> +         !test_and_set_bit (port / BITS_PER_EVTCHN_WORD(d),
> +                            &vcpu_info(v, evtchn_pending_sel)) )

Similarly here.

> +    {
> +        vcpu_mark_events_pending(v);
> +    }
> +}
>[...]
> @@ -615,43 +616,10 @@ out:
>  
>  static void evtchn_set_pending(struct vcpu *v, int port)
>  {
> -    struct domain *d = v->domain;
> -    int vcpuid;
> -
> -    /*
> -     * The following bit operations must happen in strict order.
> -     * NB. On x86, the atomic bit operations also act as memory barriers.
> -     * There is therefore sufficiently strict ordering for this architecture 
> --
> -     * others may require explicit memory barriers.
> -     */
> -
> -    if ( test_and_set_bit(port, &shared_info(d, evtchn_pending)) )
> -        return;
> -
> -    if ( !test_bit        (port, &shared_info(d, evtchn_mask)) &&
> -         !test_and_set_bit(port / BITS_PER_EVTCHN_WORD(d),
> -                           &vcpu_info(v, evtchn_pending_sel)) )
> -    {
> -        vcpu_mark_events_pending(v);
> -    }
> -    
> -    /* Check if some VCPU might be polling for this event. */
> -    if ( likely(bitmap_empty(d->poll_mask, d->max_vcpus)) )
> -        return;
> +    struct evtchn *evtchn;
>  
> -    /* Wake any interested (or potentially interested) pollers. */
> -    for ( vcpuid = find_first_bit(d->poll_mask, d->max_vcpus);
> -          vcpuid < d->max_vcpus;
> -          vcpuid = find_next_bit(d->poll_mask, d->max_vcpus, vcpuid+1) )
> -    {
> -        v = d->vcpu[vcpuid];
> -        if ( ((v->poll_evtchn <= 0) || (v->poll_evtchn == port)) &&
> -             test_and_clear_bit(vcpuid, d->poll_mask) )
> -        {
> -            v->poll_evtchn = 0;
> -            vcpu_unblock(v);
> -        }
> -    }
> +    evtchn = evtchn_from_port(v->domain, port);
> +    evtchn_port_set_pending(v, evtchn);

I know it's a matter of taste, but having a variable that's used
just once is usually done only whether otherwise the code
would get much harder to read. Similarly further down.

Jan