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

[Zhi Yong Wu]

On Fri, May 25, 2012 at 6:08 PM, Paolo Bonzini <address@hidden> 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