Re: [Qemu-devel] [PATCH v2] net-hub: Drop can_receive

[Stefan Hajnoczi]

On Tue, Jul 21, 2015 at 9:54 AM, Fam Zheng <address@hidden> wrote:
> On Tue, 07/21 09:36, Stefan Hajnoczi wrote:
>> On Tue, Jul 21, 2015 at 09:27:56AM +0800, Fam Zheng wrote:
>> > On Mon, 07/20 18:07, Stefan Hajnoczi wrote:
>> > > On Tue, Jul 07, 2015 at 05:41:56PM +0800, Fam Zheng wrote:
>> > > > On Tue, 07/07 09:37, Stefan Hajnoczi wrote:
>> > > > > On Tue, Jul 07, 2015 at 02:30:30PM +0800, Fam Zheng wrote:
>> > > > > > This moves the semantics from net_hub_port_can_receive to receive
>> > > > > > functions, by returning 0 if all receiving ports return 0. Also,
>> > > > > > remember to flush the source port's queue in that case.
>> > > > > >
>> > > > > > Signed-off-by: Fam Zheng <address@hidden>
>> > > > > > ---
>> > > > > >  net/hub.c | 54 
>> > > > > > +++++++++++++++++++++++++++++-------------------------
>> > > > > >  1 file changed, 29 insertions(+), 25 deletions(-)
>> > > > >
>> > > > > This patch revision doesn't take into account the special case code 
>> > > > > in
>> > > > > qemu_flush_or_purge_queued_packets(), which I mentioned in my reply 
>> > > > > to
>> > > > > the previous revision of this patch.
>> > > > >
>> > > > > The queue is now flushed twice because you've introduced
>> > > > > net_hub_port_send_cb() but qemu_flush_or_purge_queued_packets() 
>> > > > > already
>> > > > > calls net_hub_flush().
>> > > > >
>> > > > > If you want to get rid of net_hub_flush(), that's great.  But please
>> > > > > remove the duplicate code.
>> > > >
>> > > > Right, I missed that. I'll remove the duplicate and send again.
>> > >
>> > > Ping?
>> >
>> > I thought the net_hub_flush in qemu_flush_or_purge_queued_packets already 
>> > makes
>> > sure net-hub is working, so it's just about cleaning up. That's why I 
>> > deferred
>> > this to 2.5.  Am I wrong?
>>
>> My concern was that your NIC patches remove .can_receive() functions but
>> net/hub.c still relies on them via qemu_can_send_packet().
>>
>> After thinking about it more, the code should still work the same even
>> though net_hub_can_receive() is now mostly useless and should be removed
>> in QEMU 2.5.
>
> Yes. Completely removing .can_receive from the code base for 2.5 is trivial if
> the proposed fixes are correct, but it doesn't matter much now.
>
>>
>> We can leave this patch for now.
>
> OK, so net-hub is cleared, please proceed with merging the bulk .can_receive
> series, or if there are other issues, revert the qemu_set_fd_handler .can_send
> patches and I'll redo it in 2.5.

By the way, as part of getting rid of qemu_can_send_packet() it would
be good to simplify things so all logic whether to queue or deliver is
in one place.  Right now it's spread across qemu_deliver_packet(),
qemu_can_send_packet(), and qemu_send_packet_async_with_flags().

We didn't notice buggy NICs without flush calls previously because
net/net.c uses a mixture of "polling" (checking conditions for each
packet) and "blocking" (setting receive_disabled and only clearing it
upon flush).  Things seemed to work due to the polling code paths.

When we started hitting the blocking code path it became apparent that
flushes were missing.

Stefan