>From: O Mahony, Billy >Sent: Wednesday, August 16, 2017 5:16 PM >To: Kavanagh, Mark B <[email protected]>; [email protected] >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 <[email protected]>; [email protected] >> 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 <[email protected]>; [email protected] >> > Subject: RE: [ovs-dev] [PATCH 1/4] netdev: Add set_ingress_sched to >> > netdev api >> > >> > >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? >> [[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 >> > >[email protected] >> > >https://mail.openvswitch.org/mailman/listinfo/ovs-dev _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
