[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH 2/8] evtchn: refactor low-level event channel port ops
On 15/08/13 15:05, Jan Beulich wrote: >>>> On 09.08.13 at 20:08, David Vrabel <david.vrabel@xxxxxxxxxx> 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 _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx http://lists.xen.org/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |