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.mah...@intel.com>; d...@openvswitch.org > Subject: RE: [ovs-dev] [PATCH 1/4] netdev: Add set_ingress_sched to netdev > api > > >From: ovs-dev-boun...@openvswitch.org > >[mailto:ovs-dev-boun...@openvswitch.org] > >On Behalf Of Billy O'Mahony > >Sent: Thursday, July 20, 2017 5:11 PM > >To: d...@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.mah...@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 > >d...@openvswitch.org > >https://mail.openvswitch.org/mailman/listinfo/ovs-dev _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev