Re: Netback vif reference count mismatching in latest 3.11 kernels

[Tomasz Wroblewski <tomasz.wroblewski@citrix.com>]

On 11/27/2013 03:41 PM, Wei Liu wrote:
> On Wed, Nov 27, 2013 at 03:07:09PM +0100, Tomasz Wroblewski wrote:
>> Hi,
>>
>> After update of our network backend vm kernel to 3.11.9 I'm seeing
>> trouble with netback vif close which seem related to the recent
>> changes which separated vif disconnect and free; It seems that now
>> multiple disconnect/connect cycles can happen without freeing and
>> reallocing the netdev in the processes, which confuses the vif
>> refcount.
>>
>> vif refcount is initialized to 1 in xenvif_alloc. Then first
>> xenvif_disconnect brings it back to 0, instead of 1 which would seem
>> more reasonable (since its initialized to 1 in xenvif_alloc i would
>> expect it to not be dropped to 0 until xenvif_free). Second
>> xenvif_disconnect brings it to -1 and hangs. For us (xenclient XT)
>> this happens when we hibernate linux guest, since linux hibernate is
>> a complex beast which transitions the drivers to between
>> close/connected states multiple times (i.e. first it suspends/closes
>> the drivers to take memory snapshot, then resumes/reconnects the
>> drivers to the actual writing of hibernate image to disk, then
>> finally it closes them again to shutdown the system)
>>
>
> Can you illustrate a graph of the whole process? I'm not very clear of
> the whole cycle.
>
> There's a xenvif_get in xenvif_connect, which increases refcnt by 1,
> that should corresponds to the atomic_dec in xenvif_disconnect, right?
>

Not if I'm reading the code right. I think the atomic_dec in xenvif_disconnect corresponds to atomic_set(..., 1) in xenvif_alloc, and 
xenvif_get() in xenvif_connect corresponds to xenvif_put in xenvif_carrier_off() (which is called at top of xenvif_disconnect).

Therefore xenvif_disconnect() decrements the refcnt twice whilst xenvif_connect() increments it once, which results in negative refcnt after 
one cycle. The graph I'm seeing looks like this:

      | guest vm boots
      |
      v
netback:xenvif_alloc() refcnt after=1
      |
      v
netback:xenvif_connect() refcnt after=2
      |
      v
    ..... stuff
      |
      | guest hibernate starts and suspends netfront (netfront goes to Closed xenbus state)
      |
      v
netback:xenvif_disconnect() refcnt after=0
      |
      | hibernate takes memory snapshot and resumes netfront afterwards (which will go thru initialising, connected etc states)
      |
      v
netback:xenvif_connect() refcnt after=1
      |
      | hibernate suspends netfront again (netfront goes to Closed xenbus state)
      |
      v
netback:xenvif_disconnect() refcnt=-1
      |
      v
   netback stuck
      |
      v
netback:xenvif_free() never happens

>> I've hacked the attached patch which fixes it (for us), is the approach taken there correct/upstreamable/reasonable? It does the following
>>
>>      * reset tx_irq to 0 after unbinding the irqs on disconnect -
>> because xenvif_connect tests for it being 0 and will not reconnect
>> if it's not reset
>>      * reacquire one reference to vif in disconnect(). This is because the reference
>>        vif should be 1, as initialized in xenvif_alloc(), until the vif is freed. Otherwise multiple disconne
>>        and cause a hang. I imagine alternate way of fixing this could be to use "0" as the default
>>        refcnt in xenvif_alloc()
>>
>
> You mean the numbers of connect's and disconnect's don't match? Even
> after you reset tx_irq to 0?
>
The number of connects and disconnect matches, but the amounts of refcnt changes do not seem to be symmetric in these functions. And yeah, 
the kernel I'm using already has Paul's "robustness" fix and subsequent zombie fix by David Vrabel.