From mboxrd@z Thu Jan 1 00:00:00 1970 From: David Vrabel Subject: Re: [PATCH] evtchn/fifo: don't corrupt queues if an old tail is linked Date: Mon, 9 Dec 2013 14:43:11 +0000 Message-ID: <52A5D6FF.5060509@citrix.com> References: <1386351512-16388-1-git-send-email-david.vrabel@citrix.com> <1386351512-16388-2-git-send-email-david.vrabel@citrix.com> <52A59C43020000780010B473@nat28.tlf.novell.com> <52A5AE52.8020601@citrix.com> <52A5C3E9020000780010B647@nat28.tlf.novell.com> <52A5BE06.9020807@citrix.com> <52A5CF49020000780010B6C1@nat28.tlf.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta14.messagelabs.com ([193.109.254.103]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1Vq23N-0002wY-T0 for xen-devel@lists.xenproject.org; Mon, 09 Dec 2013 14:43:22 +0000 In-Reply-To: <52A5CF49020000780010B6C1@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 List-Id: xen-devel@lists.xenproject.org On 09/12/13 13:10, Jan Beulich wrote: >>>> On 09.12.13 at 13:56, David Vrabel wrote: >> On 09/12/13 12:21, Jan Beulich wrote: >>>>>> On 09.12.13 at 12:49, David Vrabel wrote: >>>> On 09/12/13 09:32, Jan Beulich wrote: >>>>>>>> On 06.12.13 at 18:38, David Vrabel wrote: >>>>>> --- a/xen/include/xen/sched.h >>>>>> +++ b/xen/include/xen/sched.h >>>>>> @@ -98,6 +98,8 @@ struct evtchn >>>>>> } u; >>>>>> u8 priority; >>>>>> u8 pending:1; >>>>>> + u16 last_vcpu_id; >>>>>> + u8 last_priority; >>>>> >>>>> Is it really correct for these two new fields to remain uninitialized >>>>> until evtchn_fifo_set_pending() would get run the first time (and >>>>> hence thinking there was a move this first time through)? >>>> >>>> They're initialized to zero and I think this is fine. The code as-is is >>>> simpler than having to special case events that have never been on a queue. >>> >>> I'm not asking to add a special case, I'm only asking to initialize all >>> fields correctly. Just like you ought to set up ->priority, you >>> likely ought to set up the two new fields. >> >> It's not clear how you think they're not initialized. They're >> initialized to zero when the evtchn is allocated and then they must only >> be set in evtchn_fifo_set_pending() when they move to a new queue. > > My primary concern is with them being zero (and hence out of sync > with the real values that things start out with), there may be subtle > corruption later on. Secondary is that - as said - this would at least > trigger one unnecessary move in evtchn_fifo_set_pending(). (0, 0) is still a valid queue and it is always safe to do: if ( old_q->tail == port ) old_q->tail = 0 so I'm not seeing any risk of subtle corruption anywhere. An unnecessary move once per port is hardly expensive so not something I would introduce complexity in the common case to avoid. >> Do you think they should be initialized when an event is (re)bound? >> Because this would be broken as an unbound event might be an old tail. > > But if you don't do this, then you _require_ a set-priority operation, > yet that one's necessarily non-atomic with the bind. Newly created > event channels should start out at the default priority irrespective > of what the underlying tracking structure in the hypervisor was > used for before. Xen can only move an event between queues if that event isn't on a queue. It is also not notified when an event is removed from a queue. The guest can ensure a predictable state by only unbinding events that are not currently on a queue. e.g., /* prevent it becoming LINKED. */ set_bit(word, MASKED) /* wait for interrupt handlers to drain event from its queue. */ while (test_bit(word, LINKED)) ; /* Unlinked and masked, safe to unbind. If this port is bound again it will becoming pending on the correct new queue. */ unbind() There doesn't need to be anything added to Xen to support this. The guest may need to defer to wait and unbind to a work queue or similar. David