Hi Mark,
> -----Original Message----- > From: O Mahony, Billy > Sent: Wednesday, August 16, 2017 4:53 PM > To: Kavanagh, Mark B <mark.b.kavan...@intel.com>; d...@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.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. [[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 > > >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
