[Xen-devel] Re: netback commit history

[Ian Campbell <Ian.Campbell@xxxxxxxxxxxxx>]

On Tue, 2011-09-20 at 13:38 +0100, Jan Beulich wrote:
> >>> On 20.09.11 at 14:10, "Jan Beulich" <JBeulich@xxxxxxxx> wrote:
> >>>> On 20.09.11 at 13:26, Ian Campbell <Ian.Campbell@xxxxxxxxxxxxx> wrote:
> >> On Tue, 2011-09-20 at 12:04 +0100, Jan Beulich wrote:
> >>> One thing I wonder about in this context is whether the
> >>> netif_stop_queue() call from xenvif_close() shouldn't happen before
> >>> xenvif_down() (not the least for reasons of symmetry with
> >>> xenvif_open()).
> >> 
> >> I seem to recall looking at that too, it was the same in the old kernels
> >> too and I didn't know why so I avoided touching it (I was doing too much
> >> other cleanup at the time to risk it).
> > 
> > After looking further I don't think that would help (though I still think
> > it would be more correct), as netif_stop_queue() basically evaluates
> > to a set_bit() without any other locks taken. So it's completely
> > asynchronous wrt dev_hard_start_xmit() (and its callers, which are
> > the ones looking at the bit with HARD_TX_LOCK() carried out).
> 
> Which in turn suggests that the upstream driver isn't safe either:
> There's nothing preventing vif->netbk from becoming NULL between
> the early check in xenvif_start_xmit() and its actual use in
> xen_netbk_queue_tx_skb(). I think vif->netbk needs to be latched
> into a local variable, checked against NULL, and passed instead of
> vif to xen_netbk_queue_tx_skb().

The xmit function is called with txq->_xmit_lock held.

I think we should be calling netif_tx_disable at some point before
making vif->netbk == NULL. Need to figure out where (or if we are
already doing it indirectly)

Ian.



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