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

[David Vrabel <david.vrabel@citrix.com>]

On 15/08/13 15:05, Jan Beulich wrote:
>>>> 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.

I think it is fine for the ABI specific hooks to make calls to common
code but tried this anyway and I don't think it's an improvement.

The set_pending has to return three different states:

1. Do nothing.
2. Mark vcpu pending
3. Mark vcpu pending and check pollers.

I tried a couple of ways of doing this but they all look ugly with extra
branches with an interface that's less clear.

e.g.,

static bool_t evtchn_2l_set_pending(struct vcpu *v,
    struct evtchn *evtchn)
{
    struct domain *d = v->domain;
    unsigned port = evtchn->port;
    unsigned action = 0;

    if ( test_and_set_bit(port, &shared_info(d, evtchn_pending)) )
        return action;

    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)) )
    {
        action |= MARK_PENDING;
    }
    action |= CHECK_POLLERS
    return action;
}
[...]
static void evtchn_set_pending(struct vcpu *v, int port)
{
    struct domain *d = v->domain;
    unsigned action;

    action = evtchn_port_set_pending(v, evtchn_from_port(d, port));
    if ( action & MARK_PENDING )
        vcpu_mark_pending(v);
    if ( action & CHECK_PENDING )
        evtchn_check_pollers(d, port);
}

Which just looks bleah to me.

I also tried:

static bool_t evtchn_2l_set_pending(struct vcpu *v,
    struct evtchn *evtchn)
{
    struct domain *d = v->domain;
    unsigned port = evtchn->port;

    if ( test_and_set_bit(port, &shared_info(d, evtchn_pending)) )
        return 0;

    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);
    }
    return 1;
}
[...]
static void evtchn_set_pending(struct vcpu *v, int port)
{
    struct domain *d = v->domain;

    if (evtchn_port_set_pending(v, evtchn_from_port(d, port)))
        evtchn_check_pollers(d, port);
}

But this means we can't move the vcpu_mark_events_pending() out of the
unmask hook because the FIFO unmask calls set_pending which calls
vcpu_mark_events_pending().

Any other suggestions or is the original fine as-is?

David