[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v3 16/16] hub: add the support for hub own flow
From: |
Paolo Bonzini |
Subject: |
Re: [Qemu-devel] [PATCH v3 16/16] hub: add the support for hub own flow control |
Date: |
Fri, 25 May 2012 12:08:16 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:12.0) Gecko/20120430 Thunderbird/12.0.1 |
Il 25/05/2012 09:48, Zhi Yong Wu ha scritto:
>>> static ssize_t net_hub_receive(NetHub *hub, NetHubPort *source_port,
>>> const uint8_t *buf, size_t len)
>>> {
>>> NetHubPort *port;
>>> + ssize_t ret = 0;
>>>
>>> QLIST_FOREACH(port, &hub->ports, next) {
>>> if (port == source_port) {
>>> continue;
>>> }
>>>
>>> - qemu_send_packet(&port->nc, buf, len);
>>> + ret = qemu_send_packet_async(&port->nc, buf, len,
>>> + net_hub_receive_completed);
>>
>> Just increment nr_packets here:
>>
>> ret = qemu_send_packet_async
>> if (ret == 0) {
>> port->nr_packets++;
>> }
> This is wrong, if you check the code, sent_cb is only called when the
> send queue is not empty. That is, sent_cb is used for those enqueued
> packets. For those packets which aren't enqueued, The counter will be
> not decreased.
It will also not be incremented, because I'm checking for ret == 0.
>>> }
>>> - return len;
>>> + return ret;
>>
>> You can return len here. In fact returning ret is wrong because the
>> value comes from a random port (the last one).
> If the return value from the last port doesn't equal to len, you let
> this function return len, it will be also wrong.
But that's the whole point of implementing flow control. We return len
because we _did_ process the packet; it is now in the port's queues.
However, can_receive will not admit new packets until all ports have
processed the previous one, so that all ports advance to new packets at
the same time.
>>
>>> }
>>>
>>> static ssize_t net_hub_receive_iov(NetHub *hub, NetHubPort *source_port,
>>> @@ -65,7 +84,8 @@ static ssize_t net_hub_receive_iov(NetHub *hub,
>>> NetHubPort *source_port,
>>> continue;
>>> }
>>>
>>> - ret = qemu_sendv_packet(&port->nc, iov, iovcnt);
>>> + ret = qemu_sendv_packet_async(&port->nc, iov, iovcnt,
>>> + net_hub_receive_completed);
>>
>> Same here (increment nr_packets)
>>
>>> }
>>> return ret;
>>
>> Same here (return len).
> No, it has no such variable called as len, I think that here should
> return ret, not len.
> Do you think that it is necessary to calc len by iov and viocnt?
Yes, for the same reason as above. Returning "ret" for a random port
(the last one) does not make sense! But you could just punt: do not
implement net_hub_receive_iov at all...
>> But I think you need to implement this on the hub rather than on the
>> port, and return true only if port->nr_packets == 0 for all ports.
> Can you explain why to need based on hub, not port?
Because the purpose of the counter is to do flow control _on the hub_.
The ports can do their flow control just as well, but the hub has to
reconcile the decisions of the ports.
Taking your example from another message:
> e.g. guest <---> hubport1 - hubport2 <--> network backend.
> hubport1->nr_packets == 0 mean if guest can send packet through
> hubport1 to outside.
> while hubport2->nr_packets == 0 mean if network backend can send
> packet through hubport1 to guest.
> Their direction is different.
> So i don't understand why to need
> "port->nr_packets == 0 for all ports"?
For simplicity. Yes, this means hubs will be half-duplex. In practice
I don't think you need to care.
If you want to make it full-duplex, you can keep the per-port counter
and in can_receive check if all ports except this one has a zero
nr_packets count. In other words, your can_receive method is backwards:
a port can receive a packet if all of its sibling ports are ready to
receive it.
Don't think of it in terms of "directions". It is not correct, because
it is a star topology. Think of it in terms of where the packets enter
the hub, and where they are forwarded to.
>> Probably you can move nr_packets to the hub itself rather than the port?
> I think that the counter brings a lot of issues.
I said already that it's not *necessary*. You're free to find another
solution. Removing TODO comments and leaving the problem however is not
a solution.
Paolo
- [Qemu-devel] [PATCH v3 15/16] net: determine if packets can be sent before net queue deliver packets, (continued)
- [Qemu-devel] [PATCH v3 15/16] net: determine if packets can be sent before net queue deliver packets, zwu . kernel, 2012/05/24
- [Qemu-devel] [PATCH v3 11/16] net: Rename vc local variables to nc, zwu . kernel, 2012/05/24
- [Qemu-devel] [PATCH v3 13/16] net: Make the monitor output more reasonable hub info, zwu . kernel, 2012/05/24
- [Qemu-devel] [PATCH v3 10/16] net: Rename VLANClientState to NetClientState, zwu . kernel, 2012/05/24
- [Qemu-devel] [PATCH v3 16/16] hub: add the support for hub own flow control, zwu . kernel, 2012/05/24
- Re: [Qemu-devel] [PATCH v3 16/16] hub: add the support for hub own flow control, Zhi Yong Wu, 2012/05/25
- Re: [Qemu-devel] [PATCH v3 16/16] hub: add the support for hub own flow control, Zhi Yong Wu, 2012/05/25
[Qemu-devel] [PATCH v3 12/16] net: Rename qemu_del_vlan_client() to qemu_del_net_client(), zwu . kernel, 2012/05/24
Re: [Qemu-devel] [PATCH v3 00/16] net: hub-based networking, Luiz Capitulino, 2012/05/24