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
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
