| Message ID | 1500567047-9145-2-git-send-email-billy.o.mahony@intel.com |
|---|---|
| State | Superseded |
| Headers | show |
>From: ovs-dev-bounces@openvswitch.org [mailto:ovs-dev-bounces@openvswitch.org] >On Behalf Of Billy O'Mahony >Sent: Thursday, July 20, 2017 5:11 PM >To: dev@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@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? >+ > /* 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 >dev@openvswitch.org >https://mail.openvswitch.org/mailman/listinfo/ovs-dev
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@intel.com>; dev@openvswitch.org > Subject: RE: [ovs-dev] [PATCH 1/4] netdev: Add set_ingress_sched to netdev > api > > >From: ovs-dev-bounces@openvswitch.org > >[mailto:ovs-dev-bounces@openvswitch.org] > >On Behalf Of Billy O'Mahony > >Sent: Thursday, July 20, 2017 5:11 PM > >To: dev@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@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@openvswitch.org > >https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Hi Mark, > -----Original Message----- > From: O Mahony, Billy > Sent: Wednesday, August 16, 2017 4:53 PM > To: Kavanagh, Mark B <mark.b.kavanagh@intel.com>; dev@openvswitch.org > Subject: RE: [ovs-dev] [PATCH 1/4] netdev: Add set_ingress_sched to netdev > api > > 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@intel.com>; dev@openvswitch.org > > Subject: RE: [ovs-dev] [PATCH 1/4] netdev: Add set_ingress_sched to > > netdev api > > > > >From: ovs-dev-bounces@openvswitch.org > > >[mailto:ovs-dev-bounces@openvswitch.org] > > >On Behalf Of Billy O'Mahony > > >Sent: Thursday, July 20, 2017 5:11 PM > > >To: dev@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@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. [[BO'M]] This fn gets modified substantially later in the patch set so it's less confusing to deal with it in the later one. > > > > >+ 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@openvswitch.org > > >https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>From: O Mahony, Billy >Sent: Wednesday, August 16, 2017 5:16 PM >To: Kavanagh, Mark B <mark.b.kavanagh@intel.com>; dev@openvswitch.org >Subject: RE: [ovs-dev] [PATCH 1/4] netdev: Add set_ingress_sched to netdev api > >Hi Mark, Hey Billy, Apologies for the delayed response - comments inline as usual. Thanks, Mark > >> -----Original Message----- >> From: O Mahony, Billy >> Sent: Wednesday, August 16, 2017 4:53 PM >> To: Kavanagh, Mark B <mark.b.kavanagh@intel.com>; dev@openvswitch.org >> Subject: RE: [ovs-dev] [PATCH 1/4] netdev: Add set_ingress_sched to netdev >> api >> >> 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@intel.com>; dev@openvswitch.org >> > Subject: RE: [ovs-dev] [PATCH 1/4] netdev: Add set_ingress_sched to >> > netdev api >> > >> > >From: ovs-dev-bounces@openvswitch.org >> > >[mailto:ovs-dev-bounces@openvswitch.org] >> > >On Behalf Of Billy O'Mahony >> > >Sent: Thursday, July 20, 2017 5:11 PM >> > >To: dev@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@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. Gotcha >> > >> > >+ >> > > /* 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. Yes, it's initialized to NULL in a later patch alright; best to introduce the check nonetheless (safety first and all that). >> > >> > > 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. >[[BO'M]] This fn gets modified substantially later in the patch set so it's >less confusing to deal with it in the later one. No problem. >> > >> > >+ 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. priority_rxq is set in dpdk_apply_ingress_scheduling though, right? (from patch #2): <snip> /* Mark the appropriate q as prioritized */ dev->up.priority_rxq = priority_q_id; </snip> >> > >> > > >> > > 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@openvswitch.org >> > >https://mail.openvswitch.org/mailman/listinfo/ovs-dev
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; + /* 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); 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); + 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) /* 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; 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);
Passes ingress_sched config item from other_config column of Interface table to the netdev. Signed-off-by: Billy O'Mahony <billy.o.mahony@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(+)