[ovs-dev] [PATCH 1/4] netdev: Add set_ingress_sched to netdev api

O Mahony, Billy billy.o.mahony at intel.com
Wed Aug 16 15:53:00 UTC 2017


Hi Mark,

I'm continuing with rework/rebase. Some comments below.

> -----Original Message-----
> From: Kavanagh, Mark B
> Sent: Friday, August 4, 2017 3:49 PM
> To: O Mahony, Billy <billy.o.mahony at intel.com>; dev at openvswitch.org
> Subject: RE: [ovs-dev] [PATCH 1/4] netdev: Add set_ingress_sched to netdev
> api
> 
> >From: ovs-dev-bounces at openvswitch.org
> >[mailto:ovs-dev-bounces at openvswitch.org]
> >On Behalf Of Billy O'Mahony
> >Sent: Thursday, July 20, 2017 5:11 PM
> >To: dev at openvswitch.org
> >Subject: [ovs-dev] [PATCH 1/4] netdev: Add set_ingress_sched to netdev
> >api
> >
> >Passes ingress_sched config item from other_config column of Interface
> >table to the netdev.
> 
> 
> Hi Billy,
> 
> Thanks for the patch - some review comments inline.
> 
> Cheers,
> Mark
> 
> >
> >Signed-off-by: Billy O'Mahony <billy.o.mahony at intel.com>
> >---
> > lib/netdev-bsd.c      |  1 +
> > lib/netdev-dpdk.c     | 19 +++++++++++++++++++
> > lib/netdev-dummy.c    |  1 +
> > lib/netdev-linux.c    |  1 +
> > lib/netdev-provider.h | 10 ++++++++++
> > lib/netdev-vport.c    |  1 +
> > lib/netdev.c          | 22 ++++++++++++++++++++++
> > lib/netdev.h          |  1 +
> > vswitchd/bridge.c     |  2 ++
> > 9 files changed, 58 insertions(+)
> >
> >diff --git a/lib/netdev-bsd.c b/lib/netdev-bsd.c index 6cc83d3..eadf7bf
> >100644
> >--- a/lib/netdev-bsd.c
> >+++ b/lib/netdev-bsd.c
> >@@ -1509,6 +1509,7 @@ netdev_bsd_update_flags(struct netdev
> *netdev_,
> >enum netdev_flags off,
> >     netdev_bsd_get_etheraddr,                        \
> >     netdev_bsd_get_mtu,                              \
> >     NULL, /* set_mtu */                              \
> >+    NULL, /* set_ingress_sched */                    \
> >     netdev_bsd_get_ifindex,                          \
> >     netdev_bsd_get_carrier,                          \
> >     NULL, /* get_carrier_resets */                   \
> >diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c index
> >ea17b97..e74c50f 100644
> >--- a/lib/netdev-dpdk.c
> >+++ b/lib/netdev-dpdk.c
> >@@ -369,6 +369,9 @@ struct netdev_dpdk {
> >     /* If true, device was attached by rte_eth_dev_attach(). */
> >     bool attached;
> >
> >+    /* Ingress Scheduling config */
> >+    char *ingress_sched_str;
> 
> Would ingress_sched_cfg be more apt?
[[BO'M]] I find it useful to have the hint that this is a (human readable) string as opposed to a struct.
> 
> >+
> >     /* In dpdk_list. */
> >     struct ovs_list list_node OVS_GUARDED_BY(dpdk_mutex);
> >
> >@@ -1018,6 +1021,7 @@ netdev_dpdk_destruct(struct netdev *netdev)
> >     }
> >
> >     free(dev->devargs);
> >+    free(dev->ingress_sched_str);
> 
> There is a bug here.
> 
> In the case that a user doesn't set an ingress scheduling policy,
> netdev_dpdk's ingress_sched_str will not have been set. However, since it is
> not initialized/set to the NULL string anywhere in the code, it could
> potentially point to a random area of memory. Upon destruction of the port,
> the call to free(dev->ingress_sched_str) will free said memory, causing
> undesired behavior for any application/process using it.
> 
[[BO'M]] I'm happy to put in a checks in here -  just generally in OVS I see checks for things that ought never happen are generally not made. But maybe that is just in cycle-critical packet handling code paths. The ingress_sched_str ptr is set to NULL in common_construct() (that may be in one of the other patches) so it will either be NULL or set to a malloc'd location and should not be an issue. But TBH I'm happier with a check in front of code like this too.
> 
> >     common_destruct(dev);
> >
> >     ovs_mutex_unlock(&dpdk_mutex);
> >@@ -1941,6 +1945,20 @@ netdev_dpdk_set_mtu(struct netdev *netdev,
> int
> >mtu)  }
> >
> > static int
> >+netdev_dpdk_set_ingress_sched(struct netdev *netdev,
> >+                              const char *ingress_sched_str) {
> >+    struct netdev_dpdk *dev = netdev_dpdk_cast(netdev);
> >+
> >+    free(dev->ingress_sched_str);
> 
> As above.
> 
> >+    if (ingress_sched_str) {
> >+        dev->ingress_sched_str = xstrdup(ingress_sched_str);
> >+    }
> >+
> >+    return 0;
> >+}
> >+
> >+static int
> > netdev_dpdk_get_carrier(const struct netdev *netdev, bool *carrier);
> >
> > static int
> >@@ -3246,6 +3264,7 @@ unlock:
> >     netdev_dpdk_get_etheraddr,                                \
> >     netdev_dpdk_get_mtu,                                      \
> >     netdev_dpdk_set_mtu,                                      \
> >+    netdev_dpdk_set_ingress_sched,                            \
> >     netdev_dpdk_get_ifindex,                                  \
> >     GET_CARRIER,                                              \
> >     netdev_dpdk_get_carrier_resets,                           \
> >diff --git a/lib/netdev-dummy.c b/lib/netdev-dummy.c index
> >51d29d5..25550e0 100644
> >--- a/lib/netdev-dummy.c
> >+++ b/lib/netdev-dummy.c
> >@@ -1374,6 +1374,7 @@ netdev_dummy_update_flags(struct netdev
> *netdev_,
> >     netdev_dummy_get_etheraddr,                                 \
> >     netdev_dummy_get_mtu,                                       \
> >     netdev_dummy_set_mtu,                                       \
> >+    NULL,                       /* set_ingress_sched */         \
> >     netdev_dummy_get_ifindex,                                   \
> >     NULL,                       /* get_carrier */               \
> >     NULL,                       /* get_carrier_resets */        \
> >diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c index
> >e1d8701..a1c15fd 100644
> >--- a/lib/netdev-linux.c
> >+++ b/lib/netdev-linux.c
> >@@ -2835,6 +2835,7 @@ netdev_linux_update_flags(struct netdev
> *netdev_,
> >enum netdev_flags off,
> >     netdev_linux_get_etheraddr,                                 \
> >     netdev_linux_get_mtu,                                       \
> >     netdev_linux_set_mtu,                                       \
> >+    NULL,                       /* set_ingress_sched */         \
> >     netdev_linux_get_ifindex,                                   \
> >     netdev_linux_get_carrier,                                   \
> >     netdev_linux_get_carrier_resets,                            \
> >diff --git a/lib/netdev-provider.h b/lib/netdev-provider.h index
> >3c3c181..d41a85f 100644
> >--- a/lib/netdev-provider.h
> >+++ b/lib/netdev-provider.h
> >@@ -34,6 +34,7 @@ extern "C" {
> >
> > struct netdev_tnl_build_header_params; #define
> NETDEV_NUMA_UNSPEC
> > OVS_NUMA_UNSPEC
> >+#define NETDEV_UNSPEC (-1)
> 
> NETDEV_PRIO_RXQ_UNSPEC is more meaningful IMO.
[[BO'M]] +1
> 
> >
> > /* A network device (e.g. an Ethernet device).
> >  *
> >@@ -72,6 +73,7 @@ struct netdev {
> >      * modify them. */
> >     int n_txq;
> >     int n_rxq;
> >+    int priority_rxq;                   /* id of prioritized rxq. -1 = None
> >*/
> >     int ref_cnt;                        /* Times this devices was opened. */
> >     struct shash_node *node;            /* Pointer to element in global map.
> >*/
> >     struct ovs_list saved_flags_list; /* Contains "struct
> >netdev_saved_flags". */ @@ -408,6 +410,14 @@ struct netdev_class {
> >      * null if it would always return EOPNOTSUPP. */
> >     int (*set_mtu)(struct netdev *netdev, int mtu);
> >
> >+    /* Sets 'netdev''s ingress scheduling policy.
> >+     *
> >+     * If 'netdev' does not support the specified policy then
> >+     * this function should return EOPNOTSUPP.  This function may be set to
> >+     * null if it would always return EOPNOTSUPP. */
> >+    int (*set_ingress_sched)(struct netdev *netdev,
> >+         const char *ingress_sched_str);
> >+
> >     /* Returns the ifindex of 'netdev', if successful, as a positive number.
> >      * On failure, returns a negative errno value.
> >      *
> >diff --git a/lib/netdev-vport.c b/lib/netdev-vport.c index
> >64a3ba3..63e77ba 100644
> >--- a/lib/netdev-vport.c
> >+++ b/lib/netdev-vport.c
> >@@ -910,6 +910,7 @@ netdev_vport_get_ifindex(const struct netdev
> *netdev_)
> >     netdev_vport_get_etheraddr,                             \
> >     NULL,                       /* get_mtu */               \
> >     NULL,                       /* set_mtu */               \
> >+    NULL,                       /* set_ingress_sched */     \
> >     GET_IFINDEX,                                            \
> >     NULL,                       /* get_carrier */           \
> >     NULL,                       /* get_carrier_resets */    \
> >diff --git a/lib/netdev.c b/lib/netdev.c index 0d5fad5..3171e98 100644
> >--- a/lib/netdev.c
> >+++ b/lib/netdev.c
> >@@ -394,6 +394,7 @@ netdev_open(const char *name, const char *type,
> >struct netdev **netdevp)
> >                 /* By default enable one tx and rx queue per netdev. */
> >                 netdev->n_txq = netdev->netdev_class->send ? 1 : 0;
> >                 netdev->n_rxq = netdev->netdev_class->rxq_alloc ? 1 :
> >0;
> >+                netdev->priority_rxq = NETDEV_UNSPEC;
> 
> Would it make sense to implement a 'setter' function for netdev-
> >priority_rxq? Many netdev attributes are modified via get/set functions
[[BO'M]] The priority_rxq is not intended to be set by something outside of the netdev mechanism. It's just the process_rxq that needs to be aware of it and even then it is a read-only value. So right now I'd say not.
> 
> >
> >                 ovs_list_init(&netdev->saved_flags_list);
> >
> >@@ -958,6 +959,27 @@ netdev_mtu_is_user_config(struct netdev
> *netdev)
> >     return netdev->mtu_user_config;
> > }
> >
> >+/* Sets the Ingress Scheduling policy of 'netdev'.
> >+ *
> >+ * If successful, returns 0.  Returns EOPNOTSUPP if 'netdev' does not
> >+support
> >+ * the specified policy */
> >+int
> >+netdev_set_ingress_sched(struct netdev *netdev, const char
> >*ingress_sched_str)
> >+{
> >+    const struct netdev_class *class = netdev->netdev_class;
> >+    int error;
> >+
> >+    error = class->set_ingress_sched ?
> >+        class->set_ingress_sched(netdev, ingress_sched_str) : EOPNOTSUPP;
> >+    if (error && error != EOPNOTSUPP) {
> >+        VLOG_DBG_RL(&rl, "failed to set ingress scheduling for network " \
> >+                    "device %s: %s",
> >+                    netdev_get_name(netdev), ovs_strerror(error));
> >+    }
> >+
> >+    return error;
> >+}
> >+
> > /* Returns the ifindex of 'netdev', if successful, as a positive
> >number.  On
> >  * failure, returns a negative errno value.
> >  *
> >diff --git a/lib/netdev.h b/lib/netdev.h index 998f942..aebd666 100644
> >--- a/lib/netdev.h
> >+++ b/lib/netdev.h
> >@@ -164,6 +164,7 @@ int netdev_get_mtu(const struct netdev *, int
> >*mtup);  int netdev_set_mtu(struct netdev *, int mtu);  void
> >netdev_mtu_user_config(struct netdev *, bool);  bool
> >netdev_mtu_is_user_config(struct netdev *);
> >+int netdev_set_ingress_sched(struct netdev *, const char
> >+*ingress_sched_str);
> > int netdev_get_ifindex(const struct netdev *);  int
> >netdev_set_tx_multiq(struct netdev *, unsigned int n_txq);  enum
> >netdev_pt_mode netdev_get_pt_mode(const struct netdev *); diff --git
> >a/vswitchd/bridge.c b/vswitchd/bridge.c index 16d83c9..9113195 100644
> >--- a/vswitchd/bridge.c
> >+++ b/vswitchd/bridge.c
> >@@ -1795,6 +1795,8 @@ iface_do_create(const struct bridge *br,
> >     }
> >
> >     iface_set_netdev_mtu(iface_cfg, netdev);
> >+    netdev_set_ingress_sched(netdev,
> >+        smap_get(&iface_cfg->other_config, "ingress_sched"));
> >
> >     *ofp_portp = iface_pick_ofport(iface_cfg);
> >     error = ofproto_port_add(br->ofproto, netdev, ofp_portp);
> >--
> >2.7.4
> >
> >_______________________________________________
> >dev mailing list
> >dev at openvswitch.org
> >https://mail.openvswitch.org/mailman/listinfo/ovs-dev


More information about the dev mailing list