Re: [RFC][PATCH V2 1/3] examples/vhost: Add vswitch (generic switch) framework

[Yuanhan Liu <yuanhan.liu@linux.intel.com>]

On Tue, Sep 20, 2016 at 02:28:17PM +0530, Pankaj Chauhan wrote:
> On 9/19/2016 8:13 PM, Yuanhan Liu wrote:
> >Firstly, sorry for being late on this discussion: I just got a chance
> >to follow what you guys were talking about.
> >
> >On Tue, Sep 13, 2016 at 02:51:31PM +0800, Tan, Jianfeng wrote:
> >>>(2) we'd better not differentiate phys device and virtual
> >
> >Agreed.
> >
> >>>>device in generic framework (it's just an attribute of vswitch_port.
> >>>>
> >>>>What do you think?
> >>>
> >>>I agree with your thought that given the current API in this patchset we
> >>>should aim for making switch_worker agnostic of the port type. Ideally it
> >>>should look something like this:
> >>>
> >>>switch_worker() {
> >>>
> >>>       rx_port mask = VSWITCH_PTYPE_PHYS | VSWITCH_PTYPE_PHYS;
> >>>
> >>>       rx_port = vs_sched_rx_port(vswit_dev_g, rx_port_mask, core_id)
> >>>       rx_q = rx_port->get_rxq(vs_port, vdev, code_id);
> >>>       rx_port->do_rx(rx_port, rxq, NULL, pktss, MAX_PKT_BURST);
> >>
> >>Can we hide queues inside struct vswitch_port? I mean:
> >>For VMDQ switch, treat (port_id, queue_id) as a vswitch_port, so far you've
> >>already stored "struct vhost_dev *" into vswitch_port.priv when it's a
> >>virtual port, how about store queue_id into vswitch_port.priv when it's a
> >>physical port.
> >
> >Well, note that vhost-user also supports multiple queue; it's just
> >haven't been enabled yet. So, storing "vdev" for virtio port and
> >"queue_id" for phys port doesn't make too much sense.
> >
> >>For arp_learning switch, make (port_id, all_enabled_queues) as a
> >>vswitch_port.
> >>Summarize above two: we treat (port_id, all_enabled_queues[]) as a
> >>vswitch_port.
> >>
> >>How about it?
> >
> >Sorry, I don't quite like the idea. It's weird to use "vswitch_port + queue_id"
> >combination to represent a port. A vswitch_port should be just a port: let's
> >keep the logic that simple.
> >
> 
> We wanted to take that approach to make vhost/main.c agnostic port type and
> have common code for rx/tx processing. The current version of patchset (v2)
> takes care of multiqueue, as it calls vs_port->get_txq/get_rxq to get the
> queue on which rx/tx has to be performed. This way the underlying switch can
> decide the queue based on core_id and vs_port.
> 
> But in the v2 patchset we still bind vhost_dev to the cores, and pass it to
> vs_port->get_rxq() to get the rx_queue corresponding to vhost_dev. Jianfeng
> had suggested to remove vhost_dev to core binding, and bind vs_port to the
> cores. Creating one vswitch_port for a physical port + queue_id was a step
> in that direction, thus creating very generic code in vhost/main.c.
> 
> YLiu/Jianfeng,
> 
> Please suggest what approach we should take here? Should we keep the logic
> of binding vhost_dev to core (as in V2 patchset), thus leaving some
> intelligence about vhost_dev in vhost/main.c.
> 
> Or What other options do you suggest if we want to achieve port type
> agnostic vhost/main.c

Hi Pankaj,

Again, apologize for late response: you see I was busy ;) Besides, I
need some time to think about it.

Generally, I think your ideal proposal looks good to me (well, I don't
see the need of port mask):

    switch_worker() {
           rx_port = vs_sched_rx_port(vswit_dev_g, core_id)
           rx_q = rx_port->get_rxq(vs_port, vdev, code_id);
           rx_port->do_rx(rx_port, rxq, NULL, pktss, MAX_PKT_BURST);

           vs_lookup_n_fwd(rx_port, pkts, rx_count, rxq);
    }

The issue is, as you stated, VMDq it's bit tricky to handle. How about
the following proposal then?

We don't have to register the nic queues while VMDq is used, since a
phys queue is bond to a virtio queue in this mode. That means only
virtio queues will be scheduled.

The virtio do_rx might look like below then:

    vmdq_rx() {
            rte_eth_rx_burst(port, queue_bond_to_this_virtio_queue, ...);
            rte_vhost_enqueue_burst(...) if any;
            
            rte_vhost_dequeue_burst(...);
    }

	--yliu