Re: [Qemu-devel] [PATCH v3 16/16] hub: add the support for hub own flow control

[Paolo Bonzini <pbonzini@redhat.com>]

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