On 9/26/2016 9:42 AM, Yuanhan Liu wrote:
> Besides the VMDq proposal, I got few more comments for you.
>
> On Mon, Sep 05, 2016 at 04:24:29PM +0530, Pankaj Chauhan wrote:
>> Introduce support for a generic framework for handling of switching between
>> physical and vhost devices. The vswitch framework introduces the following
>> concept:
>>
>> 1. vswitch_dev: Vswitch device
>
> It looks a bit confusing to me, to claim it as a "device": it's neither a
> physical nic device nor a virtio net device. Something like "vswitch_unit",
> or even "vswitch" is better and enough.
>
Yes we can change it to 'vswitch' it suites better, i'll do that in v3.
>> Signed-off-by: Pankaj Chauhan <pankaj.chauhan at nxp.com>
>> ---
>> examples/vhost/Makefile | 2 +-
>> examples/vhost/main.c | 128 +++++++++--
>> examples/vhost/vswitch_common.c | 499
>> ++++++++++++++++++++++++++++++++++++++++
>> examples/vhost/vswitch_common.h | 186 +++++++++++++++
>> examples/vhost/vswitch_txrx.c | 97 ++++++++
>> examples/vhost/vswitch_txrx.h | 71 ++++++
>
> Seems that you forgot to include the file to implment all those ops for
> "switch" vswitch mode? I mean, I just see a vs_lookup_n_fwd implmentation
> of VMDq.
>
No i didn't forget to include the file but wanted to implement, get
reviewed and included (hopefully :)) the implementation of following first:
1. vswitch framework
2. vmdq implementation plugged into the vswitch framework.
After above two i am planning to send the 'software switch'
implementation in a separate patch, i hope that is fine.
>> @@ -1241,7 +1296,7 @@ static int
>> new_device(int vid)
>> {
>> int lcore, core_add = 0;
>> - uint32_t device_num_min = num_devices;
>> + uint32_t device_num_min;
>> struct vhost_dev *vdev;
>>
>> vdev = rte_zmalloc("vhost device", sizeof(*vdev), RTE_CACHE_LINE_SIZE);
>> @@ -1252,6 +1307,16 @@ new_device(int vid)
>> return -1;
>> }
>> vdev->vid = vid;
>> + device_num_min = vs_get_max_vdevs(vswitch_dev_g);
>> + RTE_LOG(INFO, VHOST_PORT, "max virtio devices %d\n", device_num_min);
>> +
>> + vs_port = vs_add_port(vswitch_dev_g, vid, VSWITCH_PTYPE_VIRTIO, vdev);
>
> Note that "vid" does not equal "port". They are two different counters
> and both start from 0. That means, you will get unexpected results from
> following piece of code ---->
>
Sorry i didn't get the inconsistency completely, please help me
understand it.
I agree both port_id and vid counters start from zero. But when we add
these as vswitch_port we'll pass different port type
(VSWITCH_PTYPE_VIRTIO or VSWITCH_PTYPE_PHYS). And while searching for
any vswitch port we use vs_port->port_id && vs_port->type as the key,
thus we'll not get confused between ports even when both have same port_id.
Can you please help me understand the inconsistency that you thought we
may have?
Thanks,
Pankaj
>> +struct vswitch_port *vs_add_port(struct vswitch_dev *vs_dev, int port_id,
>> + enum vswitch_port_type type, void *priv)
>> +{
>> + int rc = 0;
>> + struct vswitch_port *vs_port = NULL;
>> + struct vswitch_ops *vs_ops = vs_dev->ops;
>> +
>> + vs_port = vs_get_free_port(vs_dev);
>> + if (!vs_port) {
>> + RTE_LOG(DEBUG, VHOST_CONFIG, "Failed get free port in \
>> + vswitch %s\n", vs_dev->name);
>> + rc = -EBUSY;
>> + goto out;
>> + }
>> +
>> + vs_port->port_id = port_id;
>> + vs_port->type = type;
>> + vs_port->priv = priv;
>> +
>> + /* Initialize default port operations. It should be noted that
>> + * The switch ops->add_port can replace them with switch specefic
>> + * operations if required. This gives us more flexibility in switch
>> + * implementations.
>> + */
>> +
>> + switch (type) {
>> + case VSWITCH_PTYPE_PHYS:
>> + vs_port->do_tx = vs_do_tx_phys_port;
>> + vs_port->do_rx = vs_do_rx_phys_port;
>> + vs_port->get_txq = vs_get_txq_phys_port;
>> + vs_port->get_rxq = vs_get_rxq_phys_port;
>> + break;
>> + case VSWITCH_PTYPE_VIRTIO:
>> + vs_port->do_tx = vs_do_tx_virtio_port;
>> + vs_port->do_rx = vs_do_rx_virtio_port;
>> + vs_port->get_txq = vs_get_txq_virtio_port;
>> + vs_port->get_rxq = vs_get_rxq_virtio_port;
>> + break;
>> + default:
>> + RTE_LOG(DEBUG, VHOST_CONFIG, "Invalid port [id %d, type %d]",
>> + port_id, type);
>> + rc = -EINVAL;
>> + goto out;
>> + }
>> +
>> + if (vs_ops->add_port)
>> + rc = vs_ops->add_port(vs_port);
>> +
>> + if (rc)
>> + goto out;
>> +
>> + vs_port->state = VSWITCH_PSTATE_ADDED;
>> +
>> + rte_eth_macaddr_get(vs_port->port_id, &vs_port->mac_addr);
>
> <--- here.
>
> --yliu
>
>> + RTE_LOG(INFO, VHOST_PORT, "Port %u MAC: %02"PRIx8" %02"PRIx8" %02"PRIx8
>> + " %02"PRIx8" %02"PRIx8" %02"PRIx8"\n",
>> + (unsigned)port_id,
>> + vs_port->mac_addr.addr_bytes[0],
>> + vs_port->mac_addr.addr_bytes[1],
>> + vs_port->mac_addr.addr_bytes[2],
>> + vs_port->mac_addr.addr_bytes[3],
>> + vs_port->mac_addr.addr_bytes[4],
>> + vs_port->mac_addr.addr_bytes[5]);
>> +
>> + RTE_LOG(DEBUG, VHOST_CONFIG, "Added port [%d, type %d] to \
>> + vswitch %s\n", vs_port->port_id, type, vs_dev->name);
>> +out:
>> + if (rc){
>> + RTE_LOG(INFO, VHOST_CONFIG, "Failed to Add port [%d, type %d]
>> to \
>> + vswitch %s\n", port_id, type, vs_dev->name);
>> + if (vs_port)
>> + vs_free_port(vs_port);
>> + vs_port = NULL;
>> + }
>> +
>> + return vs_port;
>> +}
>
