>From: [email protected] [mailto:[email protected]] >On Behalf Of Billy O'Mahony >Sent: Thursday, July 20, 2017 5:11 PM >To: [email protected] >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 <[email protected]> >--- > 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? >+ > /* 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. > 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. > > /* 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 > > 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 >[email protected] >https://mail.openvswitch.org/mailman/listinfo/ovs-dev _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
