On Fri, May 25, 2012 at 6:08 PM, Paolo Bonzini <pbonzini@xxxxxxxxxx> wrote:
> 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.
I like the words. I sent out v4 for this patch. In new version, i
define one new rule for hub port .can_receive().
anyway, thanks a lot for your comments. Have nice weekend!!
>
> Paolo
--
Regards,
Zhi Yong Wu
--
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
