Re: [Qemu-devel] [PATCH 2/3] net: introduce lock to protect NetClientState's send_queue

[Stefan Hajnoczi <stefanha@gmail.com>]

On Tue, Mar 05, 2013 at 11:04:28AM +0800, liu ping fan wrote:
> On Tue, Mar 5, 2013 at 10:45 AM, liu ping fan <qemulist@gmail.com> wrote:
> > On Mon, Mar 4, 2013 at 10:49 PM, Stefan Hajnoczi <stefanha@gmail.com> wrote:
> >> On Sun, Mar 03, 2013 at 09:21:21PM +0800, Liu Ping Fan wrote:
> >>> From: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
> >>> @@ -307,6 +309,28 @@ static void qemu_free_net_client(NetClientState *nc)
> >>>      }
> >>>  }
> >>>
> >>> +/* exclude race with rx/tx path, flush out peer's queue */
> >>> +static void qemu_flushout_net_client(NetClientState *nc)
> >>
> >> This function detaches the peer, the name should reflect that.
> >>
> > OK.
> >>> +{
> >>> +    NetClientState *peer;
> >>> +
> >>> +    /* sync on receive path */
> >>> +    peer = nc->peer;
> >>> +    if (peer) {
> >>> +        qemu_mutex_lock(&peer->transfer_lock);
> >>> +        peer->peer = NULL;
> >>> +        qemu_mutex_unlock(&peer->transfer_lock);
> >>> +    }
> >>
> >> This is weird.  You don't lock to read nc->peer but you do lock to write
> >> peer->peer.  If you use a lock it must be used consistently.
> > Because removal is the only code path to assign  nc->peer = NULL,  so
> > the reader and updater is serial here.  But as for peer->peer, it must
> > run against sender.
> >
> The race between removers is excluded by big lock in hot-unplug path.

Please use the locks consistently instead of relying on the global mutex
or other assumptions.  The assumptions can change (plus you haven't
documented them) so these patches make the net subsystem very brittle.

The next person wishing to modify net.c would have to reverse-engineer
all the assumptions you've made and doesn't have clear examples of which
locks to use.

Stefan