Re: [Xen-devel] [PATCH 2/2] PV framebuffer

[Markus Armbruster <armbru@xxxxxxxxxx>]

Markus Armbruster <armbru@xxxxxxxxxx> writes:

> Steven Smith <sos22@xxxxxxxxx> writes:
>
> [...]
>>> >> +        if (xenfb_hotplug(xsh, vfb_backdir) < 0)
>>> >> +                goto error;
>>> >> +        if (xenfb_hotplug(xsh, vkbd_backdir) < 0)
>>> >> +                goto error;
>>> >> +
>>> >> +        if (xenfb_xs_printf(xsh, vkbd_backdir, "feature-abs-pointer", 
>>> >> "1"))
>>> >> +                goto error;
>>> >> +        if (xenfb_xs_printf(xsh, vfb_backdir, "state", "%d",
>>> >> +                            XenbusStateInitWait))
>>> >> +                goto error;
>>> >> +        if (xenfb_xs_printf(xsh, vkbd_backdir, "state", "%d",
>>> >> +                            XenbusStateInitWait))
>>> >> +                goto error;
>>> >> +
>>> > I'd probably reorder this a little to look more like this:
>>> >
>>> > (1) Set feature-abs-pointer
>>> > (2) Set state to InitWait
>>> > (3) Set hotplug status
>>> >
>>> > The only actual *required* constraint is (1) before (2), so that the
>>> > frontend doesn't initialise before we've set the feature and
>>> > potentially miss it.
>>> >
>>> > (2) before (3) is kind of nice, in that it makes sure that the backend
>>> > is ready before xend unpauses the domain, and so the frontend'll be
>>> > able to connect the first time it tries, but that's a lot less
>>> > important here than for e.g. block devices.
>>> Based on our previous discussions, I designed the startup protocol
>>> this way:
>>> 
>>>     backend                          frontend
>>>     ------------------------------------------------------------------
>>>     hotplug_status = connected
>>>     [makes xend populate xenstore, set fe and be state = Initialising]
>>>     wait for be state = Initialising
>>>       [i.e. wait for xend]
>>>     write xs: feature-abs-pointer    write xs: feature-update
>>>     be state = InitWait              fe state = Initialised
>>>     ------------------------------ sync ------------------------------
>>>     wait for fe state = Initialised  wait for be state = InitWait
>>>     ------------------------------ sync ------------------------------
>>>     read xs: feature-update          read xs: feature-abs-pointer
>>>     write xs: request-update         write xs: request-abs-pointer
>>>     be state = Connected             fe state = Connected
>>>     ------------------------------ sync ------------------------------
>>>     wait for fe state = Connected    wait for be state = Connected
>>>     ------------------------------ sync ------------------------------
>>>     read xs: request-abs-pointer     read xs: request-update
>>> 
>>> The symmetry made sense to me.
>> Ah, sorry, I wasn't clear enough.  You've got everything right after
>> the first sync line, but the bit before that isn't quite right.  Xend
>> creates xenstore area when the domain is created, and then waits until
>> hotplug-status is set before starting the domain running.  The idea is
>> that the backend driver should be watching its area of xenbus (so
>> /local/domain/0/backend/vif, say), so that it notices when the area is
>> created and creates the appropriate backend devices.  Creating the
>> backend devices triggers the hotplug scripts via some udev magic which
>> I've never quite understood, and they then e.g. connect vifs up to the
>> bridge.  Once they've finished, the backends are all ready, and so
>> it's safe to start the guest.  If you start the guest before the
>> backends are ready, you potentially have issues like your root
>> fileystem not becoming available until after the guest has booted,
>> which tends to make Linux unhappy.
>>
>> xend                         backend driver          hotplug scripts
>> Creates a new domain
>>      paused
>> Creates backend area
>>                              Notices new backend
>>                                      area, creates
>>                                      backend device
>>                              Does some basic setup
>>                                      on backend device
>>                              Go to state InitWait
>>                              Kicks udev
>>                                                      Does a bit more setup
>>                                                              on backend
>>                                                              device
>>                                                      Sets hotplug-status
>> Notices hotplug status,
>>      unpauses domain
>>
>> Now, the obvious mapping of this protocol onto the PVFB would have
>> xend create the xenstore area when the guest is created and spawn the
>> backend itself.  The backend could then set hotplug-status to indicate
>> that it's ready, which would unpause the guest and things would then
>> proceed in the usual way.
>
> I coded this, and it works.

Not quite.  I fear there's a race.

My xenkbd otherend_changed callback runs twice with backend_state ==
XenbusStateConnected instead of first with XenbusStateInitWait and
then with XenbusStateConnected.

Here's what I think happens.  First how backend, xend and frontend
synchronize:


    backend               xend                  frontend
    ------------------------------------------------------------------
                          create domain paused
                              create xenstore,
                          fe and be state =
                              Initialising
               ________ sync _______/
              /                                
    wait for be state                           
        Initialising
    write xs: feature-*                         
    be state = InitWait
    hotplug_status =                            
        connected                               
              \________ sync _______
                                    \          
                          wait for
                              hotplug_status
                                  connected
                          unpause domain
                                    \________ sync _______
                                                          \          
                                                write xs: feature-*
                                                fe state = Initialised
              \___________________ sync __________________/
              /                                           \
    wait for fe state                           wait for be state
        Initialised                                 InitWait
    read xs: feature-*                          read xs: feature-*
    write xs: request-*                         write xs: request-*
    be state = Connected                        fe state = Connected
              \___________________ sync __________________/
              /                                           \
    wait for fe state                           wait for be state
        Connected                                   Connected
    read xs: request-*                          read xs: request-*

Except the frontend doesn't actually wait for be state, it runs a
callback (otherend_changed) when the state changes.

Now, when the this callback in the frontend is delayed sufficiently,
the order of events could be:

    frontend                                    backend
    ------------------------------------------------------------------
                             [...]
    be state = InitWait
    hotplug_status =
        connected
                             [...]
                                                fe state = Initialised
    wait for fe state
        Initialised
        completes
    read xs: feature-*  
    write xs: request-* 
    be state = Connected
                                                otherend_changed runs
                                                    and sees be state
                                                       Connected
                                                    (actual trigger
                                                        was InitWait)
                                                otherend_changed runs
                                                    and sees be state
                                                       Connected

How do you want me to deal with this?

Note the funny code in drivers/xen/blkback/xenbus.c's
frontend_changed:

        case XenbusStateInitialised:
        case XenbusStateConnected:
                /* Ensure we connect even when two watches fire in 
                   close successsion and we miss the intermediate value 
                   of frontend_state. */

If this is the right thing to do there, why isn't it done elsewhere?

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel