Re: [PATCH v8 2/3] xen/events: rework fifo queue locking

[Jürgen Groß <jgross@xxxxxxxx>]

On 27.11.20 15:11, Julien Grall wrote:
> Hi Juergen,
>
> On 27/11/2020 14:05, Jürgen Groß wrote:
> > On 27.11.20 14:58, Julien Grall wrote:
> > > Hi Juergen,
> > >
> > > On 25/11/2020 10:51, Juergen Gross wrote:
> > > > -static struct evtchn_fifo_queue *lock_old_queue(const struct domain 
> > > > *d,
> > > > -                                                struct evtchn *evtchn,
> > > > -                                                unsigned long *flags)
> > > > -{
> > > > -    struct vcpu *v;
> > > > -    struct evtchn_fifo_queue *q, *old_q;
> > > > -    unsigned int try;
> > > > -    union evtchn_fifo_lastq lastq;
> > > > -
> > > > -    for ( try = 0; try < 3; try++ )
> > > > -    {
> > > > -        lastq.raw = read_atomic(&evtchn->fifo_lastq);
> > > > -        v = d->vcpu[lastq.last_vcpu_id];
> > > > -        old_q = &v->evtchn_fifo->queue[lastq.last_priority];
> > > > -
> > > > -        spin_lock_irqsave(&old_q->lock, *flags);
> > > > -
> > > > -        v = d->vcpu[lastq.last_vcpu_id];
> > > > -        q = &v->evtchn_fifo->queue[lastq.last_priority];
> > > > -
> > > > -        if ( old_q == q )
> > > > -            return old_q;
> > > > -
> > > > -        spin_unlock_irqrestore(&old_q->lock, *flags);
> > > > -    }
> > > > -
> > > > -    gprintk(XENLOG_WARNING,
> > > > -            "dom%d port %d lost event (too many queue changes)\n",
> > > > -            d->domain_id, evtchn->port);
> > > > -    return NULL;
> > > > -}
> > > > -
> > > >   static int try_set_link(event_word_t *word, event_word_t *w, 
> > > > uint32_t link)
> > > >   {
> > > >       event_word_t new, old;
> > > > @@ -190,6 +158,9 @@ static void evtchn_fifo_set_pending(struct vcpu 
> > > > *v, struct evtchn *evtchn)
> > > >       event_word_t *word;
> > > >       unsigned long flags;
> > > >       bool_t was_pending;
> > > > +    struct evtchn_fifo_queue *q, *old_q;
> > > > +    unsigned int try;
> > > > +    bool linked = true;
> > > >       port = evtchn->port;
> > > >       word = evtchn_fifo_word_from_port(d, port);
> > > > @@ -204,17 +175,67 @@ static void evtchn_fifo_set_pending(struct 
> > > > vcpu *v, struct evtchn *evtchn)
> > > >           return;
> > > >       }
> > > > +    /*
> > > > +     * Lock all queues related to the event channel (in case of a 
> > > > queue change
> > > > +     * this might be two).
> > > > +     * It is mandatory to do that before setting and testing the 
> > > > PENDING bit
> > > > +     * and to hold the current queue lock until the event has put 
> > > > into the
> > > > +     * list of pending events in order to avoid waking up a guest 
> > > > without the
> > > > +     * event being visibly pending in the guest.
> > > > +     */
> > > > +    for ( try = 0; try < 4; try++ )
> > > May I ask why the number of try is 4 rather than the original 3?
> > Oh, I think this is just a typo. OTOH it doesn't really matter.
> I agree that the number of try was likely random and therefore using a 
> different number should not matter.
> However, this is making more difficult to review the patch because this 
> is an unexplained change.
> I would prefer if this is dropped. But if you want to keep this change, 
> then it should be explained in the commit message.
Well, I could argue that there is potentially one lock more to take, so
the retry number is increased by one, too. ;-)

I think we can just switch back to 3.


Juergen

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: application/pgp-keys

Attachment: OpenPGP_signature
Description: OpenPGP digital signature