[ovs-dev] [PATCH] netdev-dpdk: add sflow support for vhost-user ports

Kavanagh, Mark B mark.b.kavanagh at intel.com
Wed May 4 12:15:25 UTC 2016


Hi Przemek,

You're right, dpdk eth ports were supported, but there was a possibility that their indexes could overlap with kernel interfaces, right?

Either way, on reflection, this isn't a major issue; I wouldn't change the wording to 'add support for dpdk ports', as vhost cuse ports aren't supported - if I've missed something though, please let me know.

Thanks,
Mark

>
>Hi Mark,
>
>It worked for dpdk_eth ports before the patch, only "dpdk0" interface was missing from the
>stats because it had ifindex 0 assigned. Should I change the subject before resubmitting the
>patch with changes suggested by you to a more generic one, for instance "netdev-dpdk: add
>sflow support for dpdk ports"?
>
>Thanks,
>Przemek
>
>-----Original Message-----
>From: Kavanagh, Mark B
>Sent: Wednesday, May 4, 2016 12:23 PM
>To: Lal, PrzemyslawX <przemyslawx.lal at intel.com>; dev at openvswitch.org
>Subject: RE: [ovs-dev] [PATCH] netdev-dpdk: add sflow support for vhost-user ports
>
>Hi Przemek,
>
>A few review comments inline.
>
>Also, the patch/commit name suggests that sFlow support is only added for vhost-user ports,
>but it's also added for dpdk_eth ports, right?
>
>Cheers,
>Mark
>
>>
>>This patch adds sFlow support for DPDK vHost-user interfaces by
>>assigning ifindex value. Values of ifindexes for vHost-user interfaces
>>start with 2000 to avoid overlapping with kernel datapath interfaces.
>>
>>Patch also fixes issue with 'dpdk0' interface being ignored by sFlow
>>agent, because of ifindex 0. Ifindexes values for physical DPDK
>>interfaces start with 1000 to avoid overlapping with kernel datapath interfaces.
>>
>>Signed-off-by: Przemyslaw Lal <przemyslawx.lal at intel.com>
>>---
>> lib/netdev-dpdk.c | 59
>>++++++++++++++++++++++++++++++++++++++++++++++++++++++-
>> 1 file changed, 58 insertions(+), 1 deletion(-)
>>
>>diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c index
>>208c5f5..707e1d2 100644
>>--- a/lib/netdev-dpdk.c
>>+++ b/lib/netdev-dpdk.c
>>@@ -115,6 +115,16 @@ static char *vhost_sock_dir = NULL;   /* Location of vhost-user sockets
>>*/
>>  */
>> #define VHOST_ENQ_RETRY_USECS 100
>>
>>+/* For DPDK ETH interfaces use ifindex values starting with 1000
>>+ * to avoid overlapping with kernel-space interfaces.
>>+ * Also starting with 0 would cause sFlow to ignore 'dpdk0' interface.
>>+ */
>>+#define DPDK_PORT_ID_TO_IFINDEX(port_id) ((port_id) + 1000)
>>+
>>+/* For DPDK vhost-user interfaces use ifindexes starting with 2000.
>>+ */
>>+#define VHOST_ID_TO_IFINDEX(port_id) ((port_id) + 2000)
>>+
>> static const struct rte_eth_conf port_conf = {
>>     .rxmode = {
>>         .mq_mode = ETH_MQ_RX_RSS,
>>@@ -149,6 +159,7 @@ enum dpdk_dev_type {  static int rte_eal_init_ret =
>>ENODEV;
>>
>> static struct ovs_mutex dpdk_mutex = OVS_MUTEX_INITIALIZER;
>>+static struct ovs_mutex vhost_mutex = OVS_MUTEX_INITIALIZER;
>>
>> /* Quality of Service */
>>
>>@@ -790,6 +801,45 @@ netdev_dpdk_vhost_cuse_construct(struct netdev *netdev)
>>     return err;
>> }
>>
>>+/* Counter for VHOST interfaces as DPDK library doesn't provide
>>+mechanism
>>+ * similar to rte_eth_dev_count for vhost-user sockets.
>>+ */
>>+static int vhost_counter OVS_GUARDED_BY(vhost_mutex) = 0;
>>+
>>+/* Array storing vhost_ids, so their ifindex can be reused after
>>+socket
>>+ * recreation.
>>+ */
>>+static char vhost_ids[1024][PATH_MAX] OVS_GUARDED_BY(vhost_mutex);
>
>Consider using a macro for the length of 'vhost_ids', e.g. VHOST_IDS_MAX_LEN - see a later
>comment on how this could be used.
>
>>+
>>+/* Simple lookup in vhost_ids array.
>>+ * On success returns id of vhost_id stored in the array,
>>+ * otherwise returns -1.
>>+ */
>>+static int
>>+netdev_dpdk_lookup_vhost_id(struct netdev_dpdk *dev)
>>+OVS_REQUIRES(vhost_mutex) {
>>+    for (int i = 0; i <= vhost_counter; i++) {
>>+        if (strncmp(vhost_ids[i], dev->vhost_id, strlen(dev->vhost_id)) == 0) {
>>+            return i;
>>+        }
>>+    }
>>+    return -1;
>>+}
>>+
>>+/* Inserts vhost_id at the first free position in the vhost_ids array.
>>+ */
>>+static void
>>+netdev_dpdk_push_vhost_id(struct netdev_dpdk *dev) {
>>+    ovs_mutex_lock(&vhost_mutex);
>>+    if (netdev_dpdk_lookup_vhost_id(dev) < 0) {
>
>You should perform a bounds check on 'vhost_counter' to ensure that it doesn't stray past the
>end of 'vhost_ids'.
>e.g. if vhost_counter < VHOST_IDS_MAX_LEN, then perform the strncpy, otherwise warn the user
>that the list is full.
>
>>+        strncpy(vhost_ids[vhost_counter++], dev->vhost_id,
>>+                strlen(dev->vhost_id));
>>+    }
>>+    ovs_mutex_unlock(&vhost_mutex);
>>+}
>>+
>>+
>
>Remove the extraneous blank line, above.
>
>> static int
>> netdev_dpdk_vhost_user_construct(struct netdev *netdev)  { @@ -825,6
>>+875,8 @@ netdev_dpdk_vhost_user_construct(struct netdev *netdev)
>>         err = vhost_construct_helper(netdev);
>>     }
>>
>>+    netdev_dpdk_push_vhost_id(dev);
>>+
>>     ovs_mutex_unlock(&dpdk_mutex);
>>     return err;
>> }
>>@@ -1775,7 +1827,12 @@ netdev_dpdk_get_ifindex(const struct netdev *netdev)
>>     int ifindex;
>>
>>     ovs_mutex_lock(&dev->mutex);
>>-    ifindex = dev->port_id;
>>+    if (dev->type == DPDK_DEV_ETH) {
>>+        ifindex = DPDK_PORT_ID_TO_IFINDEX(dev->port_id);
>>+    }
>>+    else {
>>+        ifindex = VHOST_ID_TO_IFINDEX(netdev_dpdk_lookup_vhost_id(dev));
>>+    }
>>     ovs_mutex_unlock(&dev->mutex);
>>
>>     return ifindex;
>>--
>>2.1.0
>>
>>_______________________________________________
>>dev mailing list
>>dev at openvswitch.org
>>http://openvswitch.org/mailman/listinfo/dev


More information about the dev mailing list