From mboxrd@z Thu Jan 1 00:00:00 1970 From: David Vrabel Subject: Re: [PATCH 2/8] evtchn: refactor low-level event channel port ops Date: Fri, 6 Sep 2013 15:25:19 +0100 Message-ID: <5229E5CF.8000005@citrix.com> References: <1376071720-17644-1-git-send-email-david.vrabel@citrix.com> <1376071720-17644-3-git-send-email-david.vrabel@citrix.com> <520CFC5B02000078000EC402@nat28.tlf.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta5.messagelabs.com ([195.245.231.135]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1VHwyS-0000TA-76 for xen-devel@lists.xenproject.org; Fri, 06 Sep 2013 14:25:24 +0000 In-Reply-To: <520CFC5B02000078000EC402@nat28.tlf.novell.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Jan Beulich Cc: xen-devel , Keir Fraser , Wei Liu List-Id: xen-devel@lists.xenproject.org On 15/08/13 15:05, Jan Beulich wrote: >>>> On 09.08.13 at 20:08, David Vrabel 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