Re: [Xen-devel] [PATCH net-next 1/2] xen-netback: add a vif-is-connected flag

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: David Vrabel
> Sent: 20 September 2013 15:29
> To: Wei Liu
> Cc: Paul Durrant; netdev@xxxxxxxxxxxxxxx; xen-devel@xxxxxxxxxxxxx; Ian
> Campbell
> Subject: Re: [PATCH net-next 1/2] xen-netback: add a vif-is-connected flag
> 
> On 20/09/13 14:31, Wei Liu wrote:
> > On Fri, Sep 20, 2013 at 01:56:30PM +0100, Paul Durrant wrote:
> >> Having applied my patch to separate vif disconnect and free, I ran into a
> >> BUG when testing resume from S3 with a Windows frontend because the
> vif task
> >> pointer was not cleared by xenvif_disconnect() and so a double call to this
> >> function tries to stop the thread twice.
> >> Rather than applying a point fix for that issue it seems better to 
> >> introduce
> >> a boolean to indicate whether the vif is connected or not such that
> repeated
> >> calls to either xenvif_connect() or xenvif_disconnect() behave
> appropriately.
> 
> We already have a backend state of CONNECTED/CLOSED/etc. why do we
> need
> an additional bit of state outside of this?
> 

It's not really additional state; we were essentially inferring connected-ness 
from the value of tx_irq. This patch really just removes that inference and 
created something with the intended meaning.

> Does something like this in frontend_changed() fix it?
> 

It may well do, but it's a far more invasive change and would require more 
testing. It certainly sounds like a good thing to do in the longer term.

  Paul

>       case XenbusStateClosing:
>                 switch (dev->state) {
>                 case XenbusStateClosed;
>                       break;
>               case XenbusStateConnected:
>                       disconnect_backend(dev);
>                       /* fall through */
>               default:
>                       xenbus_switch_state(dev, XenbusStateClosing);
>                       break;
>               }
>               break;
> 
>       case XenbusStateClosed:
>                 switch (dev->state) {
>                 case XenbusStateConnected;
>                       disconnect_backend(dev);
>                       /* fall through */
>               default:
>                       xenbus_switch_state(dev, XenbusStateClosed);
>                       break;
>               }
>               if (xenbus_dev_is_online(dev))
>                       break;
>               /* fall through if not online */
> 
> Can you also remove destroy_backend()?  It's not needed any more.
> 
> I'd also recommend waiting a bit for other review feedback before
> posting an updated series.
> 
> David

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel