| Message ID | 20201215045008.29598-1-jianbol@nvidia.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [ovs-dev,v2] dpif-netlink: Fix issues of the offloaded flows counter | expand |
The 12/15/2020 04:50, Jianbo Liu wrote: > The n_offloaded_flows counter is saved in dpif, and this is the first > one when ofproto is created. When flow operation is done by ovs-appctl > commands, such as, dpctl/add-flow, a new dpif is opened, and the > n_offloaded_flows in it can't be used. So, instead of using counter, > the number of offloaded flows is queried from each netdev, then sum > them up. To achieve this, a new API is added in netdev_flow_api to get > how many flows assigned to a netdev. > > In order to get better performance, this number is calculated directly > from tc_to_ufid hmap for netdev-offload-tc, because flow dumping by tc > takes much time if there are many flows offloaded. > > Fixes: af0618470507 ("dpif-netlink: Count the number of offloaded rules") > Signed-off-by: Jianbo Liu <jianbol@nvidia.com> > --- > lib/dpif-netlink.c | 9 --------- > lib/dpif.c | 22 ++++++++++++++++++++++ > lib/dpif.h | 3 ++- > lib/netdev-offload-provider.h | 4 ++++ > lib/netdev-offload-tc.c | 20 ++++++++++++++++++++ > lib/netdev-offload.c | 27 +++++++++++++++++++++++++++ > lib/netdev-offload.h | 3 +++ > ofproto/ofproto-dpif-upcall.c | 10 ++++------ > tests/system-offloads-traffic.at | 4 ++++ > 9 files changed, 86 insertions(+), 16 deletions(-) > > diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c > index 6858ba612..2f881e4fa 100644 > --- a/lib/dpif-netlink.c > +++ b/lib/dpif-netlink.c > @@ -208,7 +208,6 @@ struct dpif_netlink { > /* Change notification. */ > struct nl_sock *port_notifier; /* vport multicast group subscriber. */ > bool refresh_channels; > - struct atomic_count n_offloaded_flows; > }; > > static void report_loss(struct dpif_netlink *, struct dpif_channel *, > @@ -654,7 +653,6 @@ dpif_netlink_run(struct dpif *dpif_) > static int > dpif_netlink_get_stats(const struct dpif *dpif_, struct dpif_dp_stats *stats) > { > - struct dpif_netlink *dpif = dpif_netlink_cast(dpif_); > struct dpif_netlink_dp dp; > struct ofpbuf *buf; > int error; > @@ -680,7 +678,6 @@ dpif_netlink_get_stats(const struct dpif *dpif_, struct dpif_dp_stats *stats) > } > ofpbuf_delete(buf); > } > - stats->n_offloaded_flows = atomic_count_get(&dpif->n_offloaded_flows); > return error; > } > > @@ -2192,9 +2189,6 @@ try_send_to_netdev(struct dpif_netlink *dpif, struct dpif_op *op) > } > > err = parse_flow_put(dpif, put); > - if (!err && (put->flags & DPIF_FP_CREATE)) { > - atomic_count_inc(&dpif->n_offloaded_flows); > - } > log_flow_put_message(&dpif->dpif, &this_module, put, 0); > break; > } > @@ -2209,9 +2203,6 @@ try_send_to_netdev(struct dpif_netlink *dpif, struct dpif_op *op) > dpif_normalize_type(dpif_type(&dpif->dpif)), > del->ufid, > del->stats); > - if (!err) { > - atomic_count_dec(&dpif->n_offloaded_flows); > - } > log_flow_del_message(&dpif->dpif, &this_module, del, 0); > break; > } > diff --git a/lib/dpif.c b/lib/dpif.c > index ac2860764..7f01aa48b 100644 > --- a/lib/dpif.c > +++ b/lib/dpif.c > @@ -2018,3 +2018,25 @@ dpif_bond_stats_get(struct dpif *dpif, uint32_t bond_id, > ? dpif->dpif_class->bond_stats_get(dpif, bond_id, n_bytes) > : EOPNOTSUPP; > } > + > +int > +dpif_get_dp_offloaded_flows(struct dpif *dpif, uint64_t *n_flows) > +{ > + const char *dpif_type_str = dpif_normalize_type(dpif_type(dpif)); > + struct dpif_port_dump port_dump; > + struct dpif_port dpif_port; > + int ret, n_devs = 0; > + uint64_t nflows; > + > + *n_flows = 0; > + DPIF_PORT_FOR_EACH (&dpif_port, &port_dump, dpif) { > + ret = netdev_ports_get_n_flows(dpif_type_str, &dpif_port, &nflows); > + if (!ret) { > + *n_flows += nflows; > + } else if (ret == EOPNOTSUPP) { > + continue; > + } > + n_devs++; > + } > + return n_devs ? 0 : EOPNOTSUPP; > +} > diff --git a/lib/dpif.h b/lib/dpif.h > index 7ef148c6d..9d0d662fc 100644 > --- a/lib/dpif.h > +++ b/lib/dpif.h > @@ -429,7 +429,6 @@ struct dpif_dp_stats { > uint64_t n_missed; /* Number of flow table misses. */ > uint64_t n_lost; /* Number of misses not sent to userspace. */ > uint64_t n_flows; /* Number of flows present. */ > - uint64_t n_offloaded_flows; /* Number of offloaded flows present. */ > uint64_t n_mask_hit; /* Number of mega flow masks visited for > flow table matches. */ > uint32_t n_masks; /* Number of mega flow masks. */ > @@ -438,6 +437,8 @@ int dpif_get_dp_stats(const struct dpif *, struct dpif_dp_stats *); > > int dpif_set_features(struct dpif *, uint32_t new_features); > > +int dpif_get_dp_offloaded_flows(struct dpif *dpif, uint64_t *n_flows); > + > > /* Port operations. */ > > diff --git a/lib/netdev-offload-provider.h b/lib/netdev-offload-provider.h > index 0bed7bf61..cf859d1b4 100644 > --- a/lib/netdev-offload-provider.h > +++ b/lib/netdev-offload-provider.h > @@ -83,6 +83,10 @@ struct netdev_flow_api { > int (*flow_del)(struct netdev *, const ovs_u128 *ufid, > struct dpif_flow_stats *); > > + /* Get the number of flows offloaded to netdev. > + * Return 0 if successful, otherwise returns a positive errno value. */ > + int (*flow_get_n_flows)(struct netdev *, uint64_t *n_flows); > + > /* Initializies the netdev flow api. > * Return 0 if successful, otherwise returns a positive errno value. */ > int (*init_flow_api)(struct netdev *); > diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c > index 2a772a971..f17980a47 100644 > --- a/lib/netdev-offload-tc.c > +++ b/lib/netdev-offload-tc.c > @@ -1904,6 +1904,25 @@ netdev_tc_flow_del(struct netdev *netdev OVS_UNUSED, > return error; > } > > +static int > +netdev_tc_get_n_flows(struct netdev *netdev, uint64_t *n_flows) > +{ > + struct ufid_tc_data *data, *next; > + uint64_t total = 0; > + > + ovs_mutex_lock(&ufid_lock); > + HMAP_FOR_EACH_SAFE (data, next, tc_to_ufid_node, &tc_to_ufid) { > + if (data->netdev != netdev) { > + continue; > + } > + total++; > + } > + ovs_mutex_unlock(&ufid_lock); > + > + *n_flows = total; > + return 0; > +} > + > static void > probe_multi_mask_per_prio(int ifindex) > { > @@ -2044,5 +2063,6 @@ const struct netdev_flow_api netdev_offload_tc = { > .flow_put = netdev_tc_flow_put, > .flow_get = netdev_tc_flow_get, > .flow_del = netdev_tc_flow_del, > + .flow_get_n_flows = netdev_tc_get_n_flows, > .init_flow_api = netdev_tc_init_flow_api, > }; > diff --git a/lib/netdev-offload.c b/lib/netdev-offload.c > index 2da3bc701..696877058 100644 > --- a/lib/netdev-offload.c > +++ b/lib/netdev-offload.c > @@ -280,6 +280,17 @@ netdev_flow_del(struct netdev *netdev, const ovs_u128 *ufid, > : EOPNOTSUPP; > } > > +int > +netdev_flow_get_n_flows(struct netdev *netdev, uint64_t *n_flows) > +{ > + const struct netdev_flow_api *flow_api = > + ovsrcu_get(const struct netdev_flow_api *, &netdev->flow_api); > + > + return (flow_api && flow_api->flow_get_n_flows) > + ? flow_api->flow_get_n_flows(netdev, n_flows) > + : EOPNOTSUPP; > +} > + > int > netdev_init_flow_api(struct netdev *netdev) > { > @@ -602,6 +613,22 @@ netdev_ports_remove(odp_port_t port_no, const char *dpif_type) > return ret; > } > > +int > +netdev_ports_get_n_flows(const char *dpif_type, struct dpif_port *dpif_port, > + uint64_t *n_flows) > +{ > + struct port_to_netdev_data *data; > + int ret = EOPNOTSUPP; > + > + ovs_rwlock_rdlock(&netdev_hmap_rwlock); > + data = netdev_ports_lookup(dpif_port->port_no, dpif_type); > + if (data) { > + ret = netdev_flow_get_n_flows(data->netdev, n_flows); > + } > + ovs_rwlock_unlock(&netdev_hmap_rwlock); > + return ret; > +} > + > odp_port_t > netdev_ifindex_to_odp_port(int ifindex) > { > diff --git a/lib/netdev-offload.h b/lib/netdev-offload.h > index 4c0ed2ae8..ffd321bfc 100644 > --- a/lib/netdev-offload.h > +++ b/lib/netdev-offload.h > @@ -103,6 +103,7 @@ bool netdev_any_oor(void); > bool netdev_is_flow_api_enabled(void); > void netdev_set_flow_api_enabled(const struct smap *ovs_other_config); > bool netdev_is_offload_rebalance_policy_enabled(void); > +int netdev_flow_get_n_flows(struct netdev *netdev, uint64_t *n_flows); > > struct dpif_port; > int netdev_ports_insert(struct netdev *, const char *dpif_type, > @@ -124,6 +125,8 @@ int netdev_ports_flow_get(const char *dpif_type, struct match *match, > struct dpif_flow_stats *stats, > struct dpif_flow_attrs *attrs, > struct ofpbuf *buf); > +int netdev_ports_get_n_flows(const char *dpif_type, > + struct dpif_port *dpif_port, uint64_t *n_flows); > > #ifdef __cplusplus > } > diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c > index 19b92dfe0..75496ac6d 100644 > --- a/ofproto/ofproto-dpif-upcall.c > +++ b/ofproto/ofproto-dpif-upcall.c > @@ -175,7 +175,6 @@ struct udpif { > > /* n_flows_mutex prevents multiple threads updating these concurrently. */ > atomic_uint n_flows; /* Number of flows in the datapath. */ > - atomic_uint n_offloaded_flows; /* Number of the offloaded flows. */ > atomic_llong n_flows_timestamp; /* Last time n_flows was updated. */ > struct ovs_mutex n_flows_mutex; > > @@ -731,8 +730,6 @@ udpif_get_n_flows(struct udpif *udpif) > dpif_get_dp_stats(udpif->dpif, &stats); > flow_count = stats.n_flows; > atomic_store_relaxed(&udpif->n_flows, flow_count); > - atomic_store_relaxed(&udpif->n_offloaded_flows, > - stats.n_offloaded_flows); > ovs_mutex_unlock(&udpif->n_flows_mutex); > } else { > atomic_read_relaxed(&udpif->n_flows, &flow_count); > @@ -2904,10 +2901,10 @@ upcall_unixctl_show(struct unixctl_conn *conn, int argc OVS_UNUSED, > const char *argv[] OVS_UNUSED, void *aux OVS_UNUSED) > { > struct ds ds = DS_EMPTY_INITIALIZER; > + uint64_t n_offloaded_flows; > struct udpif *udpif; > > LIST_FOR_EACH (udpif, list_node, &all_udpifs) { > - unsigned int n_offloaded_flows; > unsigned int flow_limit; > bool ufid_enabled; > size_t i; > @@ -2919,8 +2916,9 @@ upcall_unixctl_show(struct unixctl_conn *conn, int argc OVS_UNUSED, > ds_put_format(&ds, " flows : (current %lu)" > " (avg %u) (max %u) (limit %u)\n", udpif_get_n_flows(udpif), > udpif->avg_n_flows, udpif->max_n_flows, flow_limit); > - atomic_read_relaxed(&udpif->n_offloaded_flows, &n_offloaded_flows); > - ds_put_format(&ds, " offloaded flows : %u\n", n_offloaded_flows); > + if (!dpif_get_dp_offloaded_flows(udpif->dpif, &n_offloaded_flows)) { > + ds_put_format(&ds, " offloaded flows : %ld\n", n_offloaded_flows); > + } > ds_put_format(&ds, " dump duration : %lldms\n", udpif->dump_duration); > ds_put_format(&ds, " ufid enabled : "); > if (ufid_enabled) { > diff --git a/tests/system-offloads-traffic.at b/tests/system-offloads-traffic.at > index 379a8a5e9..c03ffd598 100644 > --- a/tests/system-offloads-traffic.at > +++ b/tests/system-offloads-traffic.at > @@ -32,6 +32,8 @@ in_port(3),eth(macs),eth_type(0x0800),ipv4(frag=no), packets:9, bytes:882, used: > > AT_CHECK([ovs-appctl dpctl/dump-flows type=offloaded], [0], []) > > +AT_CHECK([ovs-appctl upcall/show | grep "offloaded flows : 0"], [1], [ignore]) With this change, "offloaded flows" will not be displayed if hw offload is disabled. > + > OVS_TRAFFIC_VSWITCHD_STOP > AT_CLEANUP > > @@ -64,5 +66,7 @@ in_port(2),eth(macs),eth_type(0x0800),ipv4(frag=no), packets:9, bytes:756, used: > in_port(3),eth(macs),eth_type(0x0800),ipv4(frag=no), packets:9, bytes:756, used:0.001s, actions:output > ]) > > +AT_CHECK([ovs-appctl upcall/show | grep -E "offloaded flows : [[1-9]]"], [0], [ignore]) It's 8 offloaded rules, but I got another value sometimes, which was caused by testing environment. So I don't check the exact value here. > + > OVS_TRAFFIC_VSWITCHD_STOP > AT_CLEANUP > -- > 2.26.2 >
On 12/15/20 7:30 AM, Jianbo Liu wrote: > The 12/15/2020 04:50, Jianbo Liu wrote: >> The n_offloaded_flows counter is saved in dpif, and this is the first >> one when ofproto is created. When flow operation is done by ovs-appctl >> commands, such as, dpctl/add-flow, a new dpif is opened, and the >> n_offloaded_flows in it can't be used. So, instead of using counter, >> the number of offloaded flows is queried from each netdev, then sum >> them up. To achieve this, a new API is added in netdev_flow_api to get >> how many flows assigned to a netdev. >> >> In order to get better performance, this number is calculated directly >> from tc_to_ufid hmap for netdev-offload-tc, because flow dumping by tc >> takes much time if there are many flows offloaded. Thanks, this looks like a cleaner solution. I didn't read the patch very carefully, but it looks OK at a quick glance. Some comments inline. Bets regards, Ilya Maximets. >> >> Fixes: af0618470507 ("dpif-netlink: Count the number of offloaded rules") >> Signed-off-by: Jianbo Liu <jianbol@nvidia.com> >> --- >> lib/dpif-netlink.c | 9 --------- >> lib/dpif.c | 22 ++++++++++++++++++++++ >> lib/dpif.h | 3 ++- >> lib/netdev-offload-provider.h | 4 ++++ >> lib/netdev-offload-tc.c | 20 ++++++++++++++++++++ >> lib/netdev-offload.c | 27 +++++++++++++++++++++++++++ >> lib/netdev-offload.h | 3 +++ >> ofproto/ofproto-dpif-upcall.c | 10 ++++------ >> tests/system-offloads-traffic.at | 4 ++++ >> 9 files changed, 86 insertions(+), 16 deletions(-) >> >> diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c >> index 6858ba612..2f881e4fa 100644 >> --- a/lib/dpif-netlink.c >> +++ b/lib/dpif-netlink.c >> @@ -208,7 +208,6 @@ struct dpif_netlink { >> /* Change notification. */ >> struct nl_sock *port_notifier; /* vport multicast group subscriber. */ >> bool refresh_channels; >> - struct atomic_count n_offloaded_flows; >> }; >> >> static void report_loss(struct dpif_netlink *, struct dpif_channel *, >> @@ -654,7 +653,6 @@ dpif_netlink_run(struct dpif *dpif_) >> static int >> dpif_netlink_get_stats(const struct dpif *dpif_, struct dpif_dp_stats *stats) >> { >> - struct dpif_netlink *dpif = dpif_netlink_cast(dpif_); >> struct dpif_netlink_dp dp; >> struct ofpbuf *buf; >> int error; >> @@ -680,7 +678,6 @@ dpif_netlink_get_stats(const struct dpif *dpif_, struct dpif_dp_stats *stats) >> } >> ofpbuf_delete(buf); >> } >> - stats->n_offloaded_flows = atomic_count_get(&dpif->n_offloaded_flows); >> return error; >> } >> >> @@ -2192,9 +2189,6 @@ try_send_to_netdev(struct dpif_netlink *dpif, struct dpif_op *op) >> } >> >> err = parse_flow_put(dpif, put); >> - if (!err && (put->flags & DPIF_FP_CREATE)) { >> - atomic_count_inc(&dpif->n_offloaded_flows); >> - } >> log_flow_put_message(&dpif->dpif, &this_module, put, 0); >> break; >> } >> @@ -2209,9 +2203,6 @@ try_send_to_netdev(struct dpif_netlink *dpif, struct dpif_op *op) >> dpif_normalize_type(dpif_type(&dpif->dpif)), >> del->ufid, >> del->stats); >> - if (!err) { >> - atomic_count_dec(&dpif->n_offloaded_flows); >> - } >> log_flow_del_message(&dpif->dpif, &this_module, del, 0); >> break; >> } >> diff --git a/lib/dpif.c b/lib/dpif.c >> index ac2860764..7f01aa48b 100644 >> --- a/lib/dpif.c >> +++ b/lib/dpif.c >> @@ -2018,3 +2018,25 @@ dpif_bond_stats_get(struct dpif *dpif, uint32_t bond_id, >> ? dpif->dpif_class->bond_stats_get(dpif, bond_id, n_bytes) >> : EOPNOTSUPP; >> } >> + >> +int >> +dpif_get_dp_offloaded_flows(struct dpif *dpif, uint64_t *n_flows) Shouldn't this be dpif_get_n_offloaded_flows? >> +{ >> + const char *dpif_type_str = dpif_normalize_type(dpif_type(dpif)); >> + struct dpif_port_dump port_dump; >> + struct dpif_port dpif_port; >> + int ret, n_devs = 0; >> + uint64_t nflows; >> + >> + *n_flows = 0; >> + DPIF_PORT_FOR_EACH (&dpif_port, &port_dump, dpif) { >> + ret = netdev_ports_get_n_flows(dpif_type_str, &dpif_port, &nflows); >> + if (!ret) { >> + *n_flows += nflows; >> + } else if (ret == EOPNOTSUPP) { >> + continue; >> + } >> + n_devs++; >> + } >> + return n_devs ? 0 : EOPNOTSUPP; >> +} >> diff --git a/lib/dpif.h b/lib/dpif.h >> index 7ef148c6d..9d0d662fc 100644 >> --- a/lib/dpif.h >> +++ b/lib/dpif.h >> @@ -429,7 +429,6 @@ struct dpif_dp_stats { >> uint64_t n_missed; /* Number of flow table misses. */ >> uint64_t n_lost; /* Number of misses not sent to userspace. */ >> uint64_t n_flows; /* Number of flows present. */ >> - uint64_t n_offloaded_flows; /* Number of offloaded flows present. */ >> uint64_t n_mask_hit; /* Number of mega flow masks visited for >> flow table matches. */ >> uint32_t n_masks; /* Number of mega flow masks. */ >> @@ -438,6 +437,8 @@ int dpif_get_dp_stats(const struct dpif *, struct dpif_dp_stats *); >> >> int dpif_set_features(struct dpif *, uint32_t new_features); >> >> +int dpif_get_dp_offloaded_flows(struct dpif *dpif, uint64_t *n_flows); >> + >> >> /* Port operations. */ >> >> diff --git a/lib/netdev-offload-provider.h b/lib/netdev-offload-provider.h >> index 0bed7bf61..cf859d1b4 100644 >> --- a/lib/netdev-offload-provider.h >> +++ b/lib/netdev-offload-provider.h >> @@ -83,6 +83,10 @@ struct netdev_flow_api { >> int (*flow_del)(struct netdev *, const ovs_u128 *ufid, >> struct dpif_flow_stats *); >> >> + /* Get the number of flows offloaded to netdev. >> + * Return 0 if successful, otherwise returns a positive errno value. */ >> + int (*flow_get_n_flows)(struct netdev *, uint64_t *n_flows); >> + >> /* Initializies the netdev flow api. >> * Return 0 if successful, otherwise returns a positive errno value. */ >> int (*init_flow_api)(struct netdev *); >> diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c >> index 2a772a971..f17980a47 100644 >> --- a/lib/netdev-offload-tc.c >> +++ b/lib/netdev-offload-tc.c >> @@ -1904,6 +1904,25 @@ netdev_tc_flow_del(struct netdev *netdev OVS_UNUSED, >> return error; >> } >> >> +static int >> +netdev_tc_get_n_flows(struct netdev *netdev, uint64_t *n_flows) >> +{ >> + struct ufid_tc_data *data, *next; >> + uint64_t total = 0; >> + >> + ovs_mutex_lock(&ufid_lock); >> + HMAP_FOR_EACH_SAFE (data, next, tc_to_ufid_node, &tc_to_ufid) { We're not modifying the hash map here, so it should be simple HMAP_FOR_EACH iterator. >> + if (data->netdev != netdev) { >> + continue; >> + } >> + total++; Maybe, it's better to turn 'if' condition to positive one and save some lines? e.g., if (data->netdev == netdev) { total++; } >> + } >> + ovs_mutex_unlock(&ufid_lock); >> + >> + *n_flows = total; >> + return 0; >> +} >> + >> static void >> probe_multi_mask_per_prio(int ifindex) >> { >> @@ -2044,5 +2063,6 @@ const struct netdev_flow_api netdev_offload_tc = { >> .flow_put = netdev_tc_flow_put, >> .flow_get = netdev_tc_flow_get, >> .flow_del = netdev_tc_flow_del, >> + .flow_get_n_flows = netdev_tc_get_n_flows, >> .init_flow_api = netdev_tc_init_flow_api, >> }; >> diff --git a/lib/netdev-offload.c b/lib/netdev-offload.c >> index 2da3bc701..696877058 100644 >> --- a/lib/netdev-offload.c >> +++ b/lib/netdev-offload.c >> @@ -280,6 +280,17 @@ netdev_flow_del(struct netdev *netdev, const ovs_u128 *ufid, >> : EOPNOTSUPP; >> } >> >> +int >> +netdev_flow_get_n_flows(struct netdev *netdev, uint64_t *n_flows) >> +{ >> + const struct netdev_flow_api *flow_api = >> + ovsrcu_get(const struct netdev_flow_api *, &netdev->flow_api); >> + >> + return (flow_api && flow_api->flow_get_n_flows) >> + ? flow_api->flow_get_n_flows(netdev, n_flows) >> + : EOPNOTSUPP; >> +} >> + >> int >> netdev_init_flow_api(struct netdev *netdev) >> { >> @@ -602,6 +613,22 @@ netdev_ports_remove(odp_port_t port_no, const char *dpif_type) >> return ret; >> } >> >> +int >> +netdev_ports_get_n_flows(const char *dpif_type, struct dpif_port *dpif_port, 'dpif_port' is here only for the 'port_no'. It's , probably, better, to just pass 'port_no' to this function directly. >> + uint64_t *n_flows) >> +{ >> + struct port_to_netdev_data *data; >> + int ret = EOPNOTSUPP; >> + >> + ovs_rwlock_rdlock(&netdev_hmap_rwlock); >> + data = netdev_ports_lookup(dpif_port->port_no, dpif_type); >> + if (data) { >> + ret = netdev_flow_get_n_flows(data->netdev, n_flows); >> + } >> + ovs_rwlock_unlock(&netdev_hmap_rwlock); >> + return ret; >> +} >> + >> odp_port_t >> netdev_ifindex_to_odp_port(int ifindex) >> { >> diff --git a/lib/netdev-offload.h b/lib/netdev-offload.h >> index 4c0ed2ae8..ffd321bfc 100644 >> --- a/lib/netdev-offload.h >> +++ b/lib/netdev-offload.h >> @@ -103,6 +103,7 @@ bool netdev_any_oor(void); >> bool netdev_is_flow_api_enabled(void); >> void netdev_set_flow_api_enabled(const struct smap *ovs_other_config); >> bool netdev_is_offload_rebalance_policy_enabled(void); >> +int netdev_flow_get_n_flows(struct netdev *netdev, uint64_t *n_flows); >> >> struct dpif_port; >> int netdev_ports_insert(struct netdev *, const char *dpif_type, >> @@ -124,6 +125,8 @@ int netdev_ports_flow_get(const char *dpif_type, struct match *match, >> struct dpif_flow_stats *stats, >> struct dpif_flow_attrs *attrs, >> struct ofpbuf *buf); >> +int netdev_ports_get_n_flows(const char *dpif_type, >> + struct dpif_port *dpif_port, uint64_t *n_flows); >> >> #ifdef __cplusplus >> } >> diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c >> index 19b92dfe0..75496ac6d 100644 >> --- a/ofproto/ofproto-dpif-upcall.c >> +++ b/ofproto/ofproto-dpif-upcall.c >> @@ -175,7 +175,6 @@ struct udpif { >> >> /* n_flows_mutex prevents multiple threads updating these concurrently. */ >> atomic_uint n_flows; /* Number of flows in the datapath. */ >> - atomic_uint n_offloaded_flows; /* Number of the offloaded flows. */ >> atomic_llong n_flows_timestamp; /* Last time n_flows was updated. */ >> struct ovs_mutex n_flows_mutex; >> >> @@ -731,8 +730,6 @@ udpif_get_n_flows(struct udpif *udpif) >> dpif_get_dp_stats(udpif->dpif, &stats); >> flow_count = stats.n_flows; >> atomic_store_relaxed(&udpif->n_flows, flow_count); >> - atomic_store_relaxed(&udpif->n_offloaded_flows, >> - stats.n_offloaded_flows); >> ovs_mutex_unlock(&udpif->n_flows_mutex); >> } else { >> atomic_read_relaxed(&udpif->n_flows, &flow_count); >> @@ -2904,10 +2901,10 @@ upcall_unixctl_show(struct unixctl_conn *conn, int argc OVS_UNUSED, >> const char *argv[] OVS_UNUSED, void *aux OVS_UNUSED) >> { >> struct ds ds = DS_EMPTY_INITIALIZER; >> + uint64_t n_offloaded_flows; >> struct udpif *udpif; >> >> LIST_FOR_EACH (udpif, list_node, &all_udpifs) { >> - unsigned int n_offloaded_flows; >> unsigned int flow_limit; >> bool ufid_enabled; >> size_t i; >> @@ -2919,8 +2916,9 @@ upcall_unixctl_show(struct unixctl_conn *conn, int argc OVS_UNUSED, >> ds_put_format(&ds, " flows : (current %lu)" >> " (avg %u) (max %u) (limit %u)\n", udpif_get_n_flows(udpif), >> udpif->avg_n_flows, udpif->max_n_flows, flow_limit); >> - atomic_read_relaxed(&udpif->n_offloaded_flows, &n_offloaded_flows); >> - ds_put_format(&ds, " offloaded flows : %u\n", n_offloaded_flows); >> + if (!dpif_get_dp_offloaded_flows(udpif->dpif, &n_offloaded_flows)) { >> + ds_put_format(&ds, " offloaded flows : %ld\n", n_offloaded_flows); This causes build failure on 32bit: ofproto/ofproto-dpif-upcall.c:2920:55: error: format ‘%ld’ expects argument of type ‘long int’, but argument 3 has type ‘uint64_t {aka long long unsigned int}’ [-Werror=format=] ds_put_format(&ds, " offloaded flows : %ld\n", n_offloaded_flows); ~~^ %lld You should use PRIu64 instead. >> + } >> ds_put_format(&ds, " dump duration : %lldms\n", udpif->dump_duration); >> ds_put_format(&ds, " ufid enabled : "); >> if (ufid_enabled) { >> diff --git a/tests/system-offloads-traffic.at b/tests/system-offloads-traffic.at >> index 379a8a5e9..c03ffd598 100644 >> --- a/tests/system-offloads-traffic.at >> +++ b/tests/system-offloads-traffic.at >> @@ -32,6 +32,8 @@ in_port(3),eth(macs),eth_type(0x0800),ipv4(frag=no), packets:9, bytes:882, used: >> >> AT_CHECK([ovs-appctl dpctl/dump-flows type=offloaded], [0], []) >> >> +AT_CHECK([ovs-appctl upcall/show | grep "offloaded flows : 0"], [1], [ignore]) > > With this change, "offloaded flows" will not be displayed if hw offload > is disabled. Maybe this could be: AT_CHECK([test $(ovs-appctl upcall/show | grep -c "offloaded flows") -eq 0 ]) ? > >> + >> OVS_TRAFFIC_VSWITCHD_STOP >> AT_CLEANUP >> >> @@ -64,5 +66,7 @@ in_port(2),eth(macs),eth_type(0x0800),ipv4(frag=no), packets:9, bytes:756, used: >> in_port(3),eth(macs),eth_type(0x0800),ipv4(frag=no), packets:9, bytes:756, used:0.001s, actions:output >> ]) >> >> +AT_CHECK([ovs-appctl upcall/show | grep -E "offloaded flows : [[1-9]]"], [0], [ignore]) > > It's 8 offloaded rules, but I got another value sometimes, which was > caused by testing environment. So I don't check the exact value here. Yeah, there could be some random traffic, since it's a system test, e.g. some ARP and ipv6 packets. If you want a precise check, you could still add an implementation for netdev_offload_dummy and add some checks to 'dpif-netdev - partial hw offload' tests in tests/dpif-netdev.at. It supports only partial offloading, but I don't think that really matters here.
The 12/16/2020 13:17, Ilya Maximets wrote: > On 12/15/20 7:30 AM, Jianbo Liu wrote: > > The 12/15/2020 04:50, Jianbo Liu wrote: > >> The n_offloaded_flows counter is saved in dpif, and this is the first > >> one when ofproto is created. When flow operation is done by ovs-appctl > >> commands, such as, dpctl/add-flow, a new dpif is opened, and the > >> n_offloaded_flows in it can't be used. So, instead of using counter, > >> the number of offloaded flows is queried from each netdev, then sum > >> them up. To achieve this, a new API is added in netdev_flow_api to get > >> how many flows assigned to a netdev. > >> > >> In order to get better performance, this number is calculated directly > >> from tc_to_ufid hmap for netdev-offload-tc, because flow dumping by tc > >> takes much time if there are many flows offloaded. > > Thanks, this looks like a cleaner solution. > I didn't read the patch very carefully, but it looks OK at a quick glance. > Some comments inline. Thanks. I will address them all in v3, please continue review it. > > Bets regards, Ilya Maximets. > > >> > >> Fixes: af0618470507 ("dpif-netlink: Count the number of offloaded rules") > >> Signed-off-by: Jianbo Liu <jianbol@nvidia.com> > >> --- > >> lib/dpif-netlink.c | 9 --------- > >> lib/dpif.c | 22 ++++++++++++++++++++++ > >> lib/dpif.h | 3 ++- > >> lib/netdev-offload-provider.h | 4 ++++ > >> lib/netdev-offload-tc.c | 20 ++++++++++++++++++++ > >> lib/netdev-offload.c | 27 +++++++++++++++++++++++++++ > >> lib/netdev-offload.h | 3 +++ > >> ofproto/ofproto-dpif-upcall.c | 10 ++++------ > >> tests/system-offloads-traffic.at | 4 ++++ > >> 9 files changed, 86 insertions(+), 16 deletions(-) > >> > >> diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c > >> index 6858ba612..2f881e4fa 100644 > >> --- a/lib/dpif-netlink.c > >> +++ b/lib/dpif-netlink.c > >> @@ -208,7 +208,6 @@ struct dpif_netlink { > >> /* Change notification. */ > >> struct nl_sock *port_notifier; /* vport multicast group subscriber. */ > >> bool refresh_channels; > >> - struct atomic_count n_offloaded_flows; > >> }; > >> > >> static void report_loss(struct dpif_netlink *, struct dpif_channel *, > >> @@ -654,7 +653,6 @@ dpif_netlink_run(struct dpif *dpif_) > >> static int > >> dpif_netlink_get_stats(const struct dpif *dpif_, struct dpif_dp_stats *stats) > >> { > >> - struct dpif_netlink *dpif = dpif_netlink_cast(dpif_); > >> struct dpif_netlink_dp dp; > >> struct ofpbuf *buf; > >> int error; > >> @@ -680,7 +678,6 @@ dpif_netlink_get_stats(const struct dpif *dpif_, struct dpif_dp_stats *stats) > >> } > >> ofpbuf_delete(buf); > >> } > >> - stats->n_offloaded_flows = atomic_count_get(&dpif->n_offloaded_flows); > >> return error; > >> } > >> > >> @@ -2192,9 +2189,6 @@ try_send_to_netdev(struct dpif_netlink *dpif, struct dpif_op *op) > >> } > >> > >> err = parse_flow_put(dpif, put); > >> - if (!err && (put->flags & DPIF_FP_CREATE)) { > >> - atomic_count_inc(&dpif->n_offloaded_flows); > >> - } > >> log_flow_put_message(&dpif->dpif, &this_module, put, 0); > >> break; > >> } > >> @@ -2209,9 +2203,6 @@ try_send_to_netdev(struct dpif_netlink *dpif, struct dpif_op *op) > >> dpif_normalize_type(dpif_type(&dpif->dpif)), > >> del->ufid, > >> del->stats); > >> - if (!err) { > >> - atomic_count_dec(&dpif->n_offloaded_flows); > >> - } > >> log_flow_del_message(&dpif->dpif, &this_module, del, 0); > >> break; > >> } > >> diff --git a/lib/dpif.c b/lib/dpif.c > >> index ac2860764..7f01aa48b 100644 > >> --- a/lib/dpif.c > >> +++ b/lib/dpif.c > >> @@ -2018,3 +2018,25 @@ dpif_bond_stats_get(struct dpif *dpif, uint32_t bond_id, > >> ? dpif->dpif_class->bond_stats_get(dpif, bond_id, n_bytes) > >> : EOPNOTSUPP; > >> } > >> + > >> +int > >> +dpif_get_dp_offloaded_flows(struct dpif *dpif, uint64_t *n_flows) > > Shouldn't this be dpif_get_n_offloaded_flows? > > >> +{ > >> + const char *dpif_type_str = dpif_normalize_type(dpif_type(dpif)); > >> + struct dpif_port_dump port_dump; > >> + struct dpif_port dpif_port; > >> + int ret, n_devs = 0; > >> + uint64_t nflows; > >> + > >> + *n_flows = 0; > >> + DPIF_PORT_FOR_EACH (&dpif_port, &port_dump, dpif) { > >> + ret = netdev_ports_get_n_flows(dpif_type_str, &dpif_port, &nflows); > >> + if (!ret) { > >> + *n_flows += nflows; > >> + } else if (ret == EOPNOTSUPP) { > >> + continue; > >> + } > >> + n_devs++; > >> + } > >> + return n_devs ? 0 : EOPNOTSUPP; > >> +} > >> diff --git a/lib/dpif.h b/lib/dpif.h > >> index 7ef148c6d..9d0d662fc 100644 > >> --- a/lib/dpif.h > >> +++ b/lib/dpif.h > >> @@ -429,7 +429,6 @@ struct dpif_dp_stats { > >> uint64_t n_missed; /* Number of flow table misses. */ > >> uint64_t n_lost; /* Number of misses not sent to userspace. */ > >> uint64_t n_flows; /* Number of flows present. */ > >> - uint64_t n_offloaded_flows; /* Number of offloaded flows present. */ > >> uint64_t n_mask_hit; /* Number of mega flow masks visited for > >> flow table matches. */ > >> uint32_t n_masks; /* Number of mega flow masks. */ > >> @@ -438,6 +437,8 @@ int dpif_get_dp_stats(const struct dpif *, struct dpif_dp_stats *); > >> > >> int dpif_set_features(struct dpif *, uint32_t new_features); > >> > >> +int dpif_get_dp_offloaded_flows(struct dpif *dpif, uint64_t *n_flows); > >> + > >> > >> /* Port operations. */ > >> > >> diff --git a/lib/netdev-offload-provider.h b/lib/netdev-offload-provider.h > >> index 0bed7bf61..cf859d1b4 100644 > >> --- a/lib/netdev-offload-provider.h > >> +++ b/lib/netdev-offload-provider.h > >> @@ -83,6 +83,10 @@ struct netdev_flow_api { > >> int (*flow_del)(struct netdev *, const ovs_u128 *ufid, > >> struct dpif_flow_stats *); > >> > >> + /* Get the number of flows offloaded to netdev. > >> + * Return 0 if successful, otherwise returns a positive errno value. */ > >> + int (*flow_get_n_flows)(struct netdev *, uint64_t *n_flows); > >> + > >> /* Initializies the netdev flow api. > >> * Return 0 if successful, otherwise returns a positive errno value. */ > >> int (*init_flow_api)(struct netdev *); > >> diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c > >> index 2a772a971..f17980a47 100644 > >> --- a/lib/netdev-offload-tc.c > >> +++ b/lib/netdev-offload-tc.c > >> @@ -1904,6 +1904,25 @@ netdev_tc_flow_del(struct netdev *netdev OVS_UNUSED, > >> return error; > >> } > >> > >> +static int > >> +netdev_tc_get_n_flows(struct netdev *netdev, uint64_t *n_flows) > >> +{ > >> + struct ufid_tc_data *data, *next; > >> + uint64_t total = 0; > >> + > >> + ovs_mutex_lock(&ufid_lock); > >> + HMAP_FOR_EACH_SAFE (data, next, tc_to_ufid_node, &tc_to_ufid) { > > We're not modifying the hash map here, so it should be simple HMAP_FOR_EACH > iterator. > > >> + if (data->netdev != netdev) { > >> + continue; > >> + } > >> + total++; > > Maybe, it's better to turn 'if' condition to positive one and save some lines? > e.g., > > if (data->netdev == netdev) { > total++; > } > > >> + } > >> + ovs_mutex_unlock(&ufid_lock); > >> + > >> + *n_flows = total; > >> + return 0; > >> +} > >> + > >> static void > >> probe_multi_mask_per_prio(int ifindex) > >> { > >> @@ -2044,5 +2063,6 @@ const struct netdev_flow_api netdev_offload_tc = { > >> .flow_put = netdev_tc_flow_put, > >> .flow_get = netdev_tc_flow_get, > >> .flow_del = netdev_tc_flow_del, > >> + .flow_get_n_flows = netdev_tc_get_n_flows, > >> .init_flow_api = netdev_tc_init_flow_api, > >> }; > >> diff --git a/lib/netdev-offload.c b/lib/netdev-offload.c > >> index 2da3bc701..696877058 100644 > >> --- a/lib/netdev-offload.c > >> +++ b/lib/netdev-offload.c > >> @@ -280,6 +280,17 @@ netdev_flow_del(struct netdev *netdev, const ovs_u128 *ufid, > >> : EOPNOTSUPP; > >> } > >> > >> +int > >> +netdev_flow_get_n_flows(struct netdev *netdev, uint64_t *n_flows) > >> +{ > >> + const struct netdev_flow_api *flow_api = > >> + ovsrcu_get(const struct netdev_flow_api *, &netdev->flow_api); > >> + > >> + return (flow_api && flow_api->flow_get_n_flows) > >> + ? flow_api->flow_get_n_flows(netdev, n_flows) > >> + : EOPNOTSUPP; > >> +} > >> + > >> int > >> netdev_init_flow_api(struct netdev *netdev) > >> { > >> @@ -602,6 +613,22 @@ netdev_ports_remove(odp_port_t port_no, const char *dpif_type) > >> return ret; > >> } > >> > >> +int > >> +netdev_ports_get_n_flows(const char *dpif_type, struct dpif_port *dpif_port, > > 'dpif_port' is here only for the 'port_no'. It's , probably, better, to just pass > 'port_no' to this function directly. > > >> + uint64_t *n_flows) > >> +{ > >> + struct port_to_netdev_data *data; > >> + int ret = EOPNOTSUPP; > >> + > >> + ovs_rwlock_rdlock(&netdev_hmap_rwlock); > >> + data = netdev_ports_lookup(dpif_port->port_no, dpif_type); > >> + if (data) { > >> + ret = netdev_flow_get_n_flows(data->netdev, n_flows); > >> + } > >> + ovs_rwlock_unlock(&netdev_hmap_rwlock); > >> + return ret; > >> +} > >> + > >> odp_port_t > >> netdev_ifindex_to_odp_port(int ifindex) > >> { > >> diff --git a/lib/netdev-offload.h b/lib/netdev-offload.h > >> index 4c0ed2ae8..ffd321bfc 100644 > >> --- a/lib/netdev-offload.h > >> +++ b/lib/netdev-offload.h > >> @@ -103,6 +103,7 @@ bool netdev_any_oor(void); > >> bool netdev_is_flow_api_enabled(void); > >> void netdev_set_flow_api_enabled(const struct smap *ovs_other_config); > >> bool netdev_is_offload_rebalance_policy_enabled(void); > >> +int netdev_flow_get_n_flows(struct netdev *netdev, uint64_t *n_flows); > >> > >> struct dpif_port; > >> int netdev_ports_insert(struct netdev *, const char *dpif_type, > >> @@ -124,6 +125,8 @@ int netdev_ports_flow_get(const char *dpif_type, struct match *match, > >> struct dpif_flow_stats *stats, > >> struct dpif_flow_attrs *attrs, > >> struct ofpbuf *buf); > >> +int netdev_ports_get_n_flows(const char *dpif_type, > >> + struct dpif_port *dpif_port, uint64_t *n_flows); > >> > >> #ifdef __cplusplus > >> } > >> diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c > >> index 19b92dfe0..75496ac6d 100644 > >> --- a/ofproto/ofproto-dpif-upcall.c > >> +++ b/ofproto/ofproto-dpif-upcall.c > >> @@ -175,7 +175,6 @@ struct udpif { > >> > >> /* n_flows_mutex prevents multiple threads updating these concurrently. */ > >> atomic_uint n_flows; /* Number of flows in the datapath. */ > >> - atomic_uint n_offloaded_flows; /* Number of the offloaded flows. */ > >> atomic_llong n_flows_timestamp; /* Last time n_flows was updated. */ > >> struct ovs_mutex n_flows_mutex; > >> > >> @@ -731,8 +730,6 @@ udpif_get_n_flows(struct udpif *udpif) > >> dpif_get_dp_stats(udpif->dpif, &stats); > >> flow_count = stats.n_flows; > >> atomic_store_relaxed(&udpif->n_flows, flow_count); > >> - atomic_store_relaxed(&udpif->n_offloaded_flows, > >> - stats.n_offloaded_flows); > >> ovs_mutex_unlock(&udpif->n_flows_mutex); > >> } else { > >> atomic_read_relaxed(&udpif->n_flows, &flow_count); > >> @@ -2904,10 +2901,10 @@ upcall_unixctl_show(struct unixctl_conn *conn, int argc OVS_UNUSED, > >> const char *argv[] OVS_UNUSED, void *aux OVS_UNUSED) > >> { > >> struct ds ds = DS_EMPTY_INITIALIZER; > >> + uint64_t n_offloaded_flows; > >> struct udpif *udpif; > >> > >> LIST_FOR_EACH (udpif, list_node, &all_udpifs) { > >> - unsigned int n_offloaded_flows; > >> unsigned int flow_limit; > >> bool ufid_enabled; > >> size_t i; > >> @@ -2919,8 +2916,9 @@ upcall_unixctl_show(struct unixctl_conn *conn, int argc OVS_UNUSED, > >> ds_put_format(&ds, " flows : (current %lu)" > >> " (avg %u) (max %u) (limit %u)\n", udpif_get_n_flows(udpif), > >> udpif->avg_n_flows, udpif->max_n_flows, flow_limit); > >> - atomic_read_relaxed(&udpif->n_offloaded_flows, &n_offloaded_flows); > >> - ds_put_format(&ds, " offloaded flows : %u\n", n_offloaded_flows); > >> + if (!dpif_get_dp_offloaded_flows(udpif->dpif, &n_offloaded_flows)) { > >> + ds_put_format(&ds, " offloaded flows : %ld\n", n_offloaded_flows); > > This causes build failure on 32bit: > ofproto/ofproto-dpif-upcall.c:2920:55: error: format ‘%ld’ expects argument of type > ‘long int’, but argument 3 has type ‘uint64_t {aka long long unsigned int}’ [-Werror=format=] > ds_put_format(&ds, " offloaded flows : %ld\n", n_offloaded_flows); > ~~^ > %lld > > You should use PRIu64 instead. > > >> + } > >> ds_put_format(&ds, " dump duration : %lldms\n", udpif->dump_duration); > >> ds_put_format(&ds, " ufid enabled : "); > >> if (ufid_enabled) { > >> diff --git a/tests/system-offloads-traffic.at b/tests/system-offloads-traffic.at > >> index 379a8a5e9..c03ffd598 100644 > >> --- a/tests/system-offloads-traffic.at > >> +++ b/tests/system-offloads-traffic.at > >> @@ -32,6 +32,8 @@ in_port(3),eth(macs),eth_type(0x0800),ipv4(frag=no), packets:9, bytes:882, used: > >> > >> AT_CHECK([ovs-appctl dpctl/dump-flows type=offloaded], [0], []) > >> > >> +AT_CHECK([ovs-appctl upcall/show | grep "offloaded flows : 0"], [1], [ignore]) > > > > With this change, "offloaded flows" will not be displayed if hw offload > > is disabled. > > Maybe this could be: > > AT_CHECK([test $(ovs-appctl upcall/show | grep -c "offloaded flows") -eq 0 ]) > > ? > > > > >> + > >> OVS_TRAFFIC_VSWITCHD_STOP > >> AT_CLEANUP > >> > >> @@ -64,5 +66,7 @@ in_port(2),eth(macs),eth_type(0x0800),ipv4(frag=no), packets:9, bytes:756, used: > >> in_port(3),eth(macs),eth_type(0x0800),ipv4(frag=no), packets:9, bytes:756, used:0.001s, actions:output > >> ]) > >> > >> +AT_CHECK([ovs-appctl upcall/show | grep -E "offloaded flows : [[1-9]]"], [0], [ignore]) > > > > It's 8 offloaded rules, but I got another value sometimes, which was > > caused by testing environment. So I don't check the exact value here. > > Yeah, there could be some random traffic, since it's a system test, e.g. some ARP > and ipv6 packets. If you want a precise check, you could still add an implementation > for netdev_offload_dummy and add some checks to 'dpif-netdev - partial hw offload' tests > in tests/dpif-netdev.at. It supports only partial offloading, but I don't think that > really matters here.
diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c index 6858ba612..2f881e4fa 100644 --- a/lib/dpif-netlink.c +++ b/lib/dpif-netlink.c @@ -208,7 +208,6 @@ struct dpif_netlink { /* Change notification. */ struct nl_sock *port_notifier; /* vport multicast group subscriber. */ bool refresh_channels; - struct atomic_count n_offloaded_flows; }; static void report_loss(struct dpif_netlink *, struct dpif_channel *, @@ -654,7 +653,6 @@ dpif_netlink_run(struct dpif *dpif_) static int dpif_netlink_get_stats(const struct dpif *dpif_, struct dpif_dp_stats *stats) { - struct dpif_netlink *dpif = dpif_netlink_cast(dpif_); struct dpif_netlink_dp dp; struct ofpbuf *buf; int error; @@ -680,7 +678,6 @@ dpif_netlink_get_stats(const struct dpif *dpif_, struct dpif_dp_stats *stats) } ofpbuf_delete(buf); } - stats->n_offloaded_flows = atomic_count_get(&dpif->n_offloaded_flows); return error; } @@ -2192,9 +2189,6 @@ try_send_to_netdev(struct dpif_netlink *dpif, struct dpif_op *op) } err = parse_flow_put(dpif, put); - if (!err && (put->flags & DPIF_FP_CREATE)) { - atomic_count_inc(&dpif->n_offloaded_flows); - } log_flow_put_message(&dpif->dpif, &this_module, put, 0); break; } @@ -2209,9 +2203,6 @@ try_send_to_netdev(struct dpif_netlink *dpif, struct dpif_op *op) dpif_normalize_type(dpif_type(&dpif->dpif)), del->ufid, del->stats); - if (!err) { - atomic_count_dec(&dpif->n_offloaded_flows); - } log_flow_del_message(&dpif->dpif, &this_module, del, 0); break; } diff --git a/lib/dpif.c b/lib/dpif.c index ac2860764..7f01aa48b 100644 --- a/lib/dpif.c +++ b/lib/dpif.c @@ -2018,3 +2018,25 @@ dpif_bond_stats_get(struct dpif *dpif, uint32_t bond_id, ? dpif->dpif_class->bond_stats_get(dpif, bond_id, n_bytes) : EOPNOTSUPP; } + +int +dpif_get_dp_offloaded_flows(struct dpif *dpif, uint64_t *n_flows) +{ + const char *dpif_type_str = dpif_normalize_type(dpif_type(dpif)); + struct dpif_port_dump port_dump; + struct dpif_port dpif_port; + int ret, n_devs = 0; + uint64_t nflows; + + *n_flows = 0; + DPIF_PORT_FOR_EACH (&dpif_port, &port_dump, dpif) { + ret = netdev_ports_get_n_flows(dpif_type_str, &dpif_port, &nflows); + if (!ret) { + *n_flows += nflows; + } else if (ret == EOPNOTSUPP) { + continue; + } + n_devs++; + } + return n_devs ? 0 : EOPNOTSUPP; +} diff --git a/lib/dpif.h b/lib/dpif.h index 7ef148c6d..9d0d662fc 100644 --- a/lib/dpif.h +++ b/lib/dpif.h @@ -429,7 +429,6 @@ struct dpif_dp_stats { uint64_t n_missed; /* Number of flow table misses. */ uint64_t n_lost; /* Number of misses not sent to userspace. */ uint64_t n_flows; /* Number of flows present. */ - uint64_t n_offloaded_flows; /* Number of offloaded flows present. */ uint64_t n_mask_hit; /* Number of mega flow masks visited for flow table matches. */ uint32_t n_masks; /* Number of mega flow masks. */ @@ -438,6 +437,8 @@ int dpif_get_dp_stats(const struct dpif *, struct dpif_dp_stats *); int dpif_set_features(struct dpif *, uint32_t new_features); +int dpif_get_dp_offloaded_flows(struct dpif *dpif, uint64_t *n_flows); + /* Port operations. */ diff --git a/lib/netdev-offload-provider.h b/lib/netdev-offload-provider.h index 0bed7bf61..cf859d1b4 100644 --- a/lib/netdev-offload-provider.h +++ b/lib/netdev-offload-provider.h @@ -83,6 +83,10 @@ struct netdev_flow_api { int (*flow_del)(struct netdev *, const ovs_u128 *ufid, struct dpif_flow_stats *); + /* Get the number of flows offloaded to netdev. + * Return 0 if successful, otherwise returns a positive errno value. */ + int (*flow_get_n_flows)(struct netdev *, uint64_t *n_flows); + /* Initializies the netdev flow api. * Return 0 if successful, otherwise returns a positive errno value. */ int (*init_flow_api)(struct netdev *); diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c index 2a772a971..f17980a47 100644 --- a/lib/netdev-offload-tc.c +++ b/lib/netdev-offload-tc.c @@ -1904,6 +1904,25 @@ netdev_tc_flow_del(struct netdev *netdev OVS_UNUSED, return error; } +static int +netdev_tc_get_n_flows(struct netdev *netdev, uint64_t *n_flows) +{ + struct ufid_tc_data *data, *next; + uint64_t total = 0; + + ovs_mutex_lock(&ufid_lock); + HMAP_FOR_EACH_SAFE (data, next, tc_to_ufid_node, &tc_to_ufid) { + if (data->netdev != netdev) { + continue; + } + total++; + } + ovs_mutex_unlock(&ufid_lock); + + *n_flows = total; + return 0; +} + static void probe_multi_mask_per_prio(int ifindex) { @@ -2044,5 +2063,6 @@ const struct netdev_flow_api netdev_offload_tc = { .flow_put = netdev_tc_flow_put, .flow_get = netdev_tc_flow_get, .flow_del = netdev_tc_flow_del, + .flow_get_n_flows = netdev_tc_get_n_flows, .init_flow_api = netdev_tc_init_flow_api, }; diff --git a/lib/netdev-offload.c b/lib/netdev-offload.c index 2da3bc701..696877058 100644 --- a/lib/netdev-offload.c +++ b/lib/netdev-offload.c @@ -280,6 +280,17 @@ netdev_flow_del(struct netdev *netdev, const ovs_u128 *ufid, : EOPNOTSUPP; } +int +netdev_flow_get_n_flows(struct netdev *netdev, uint64_t *n_flows) +{ + const struct netdev_flow_api *flow_api = + ovsrcu_get(const struct netdev_flow_api *, &netdev->flow_api); + + return (flow_api && flow_api->flow_get_n_flows) + ? flow_api->flow_get_n_flows(netdev, n_flows) + : EOPNOTSUPP; +} + int netdev_init_flow_api(struct netdev *netdev) { @@ -602,6 +613,22 @@ netdev_ports_remove(odp_port_t port_no, const char *dpif_type) return ret; } +int +netdev_ports_get_n_flows(const char *dpif_type, struct dpif_port *dpif_port, + uint64_t *n_flows) +{ + struct port_to_netdev_data *data; + int ret = EOPNOTSUPP; + + ovs_rwlock_rdlock(&netdev_hmap_rwlock); + data = netdev_ports_lookup(dpif_port->port_no, dpif_type); + if (data) { + ret = netdev_flow_get_n_flows(data->netdev, n_flows); + } + ovs_rwlock_unlock(&netdev_hmap_rwlock); + return ret; +} + odp_port_t netdev_ifindex_to_odp_port(int ifindex) { diff --git a/lib/netdev-offload.h b/lib/netdev-offload.h index 4c0ed2ae8..ffd321bfc 100644 --- a/lib/netdev-offload.h +++ b/lib/netdev-offload.h @@ -103,6 +103,7 @@ bool netdev_any_oor(void); bool netdev_is_flow_api_enabled(void); void netdev_set_flow_api_enabled(const struct smap *ovs_other_config); bool netdev_is_offload_rebalance_policy_enabled(void); +int netdev_flow_get_n_flows(struct netdev *netdev, uint64_t *n_flows); struct dpif_port; int netdev_ports_insert(struct netdev *, const char *dpif_type, @@ -124,6 +125,8 @@ int netdev_ports_flow_get(const char *dpif_type, struct match *match, struct dpif_flow_stats *stats, struct dpif_flow_attrs *attrs, struct ofpbuf *buf); +int netdev_ports_get_n_flows(const char *dpif_type, + struct dpif_port *dpif_port, uint64_t *n_flows); #ifdef __cplusplus } diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c index 19b92dfe0..75496ac6d 100644 --- a/ofproto/ofproto-dpif-upcall.c +++ b/ofproto/ofproto-dpif-upcall.c @@ -175,7 +175,6 @@ struct udpif { /* n_flows_mutex prevents multiple threads updating these concurrently. */ atomic_uint n_flows; /* Number of flows in the datapath. */ - atomic_uint n_offloaded_flows; /* Number of the offloaded flows. */ atomic_llong n_flows_timestamp; /* Last time n_flows was updated. */ struct ovs_mutex n_flows_mutex; @@ -731,8 +730,6 @@ udpif_get_n_flows(struct udpif *udpif) dpif_get_dp_stats(udpif->dpif, &stats); flow_count = stats.n_flows; atomic_store_relaxed(&udpif->n_flows, flow_count); - atomic_store_relaxed(&udpif->n_offloaded_flows, - stats.n_offloaded_flows); ovs_mutex_unlock(&udpif->n_flows_mutex); } else { atomic_read_relaxed(&udpif->n_flows, &flow_count); @@ -2904,10 +2901,10 @@ upcall_unixctl_show(struct unixctl_conn *conn, int argc OVS_UNUSED, const char *argv[] OVS_UNUSED, void *aux OVS_UNUSED) { struct ds ds = DS_EMPTY_INITIALIZER; + uint64_t n_offloaded_flows; struct udpif *udpif; LIST_FOR_EACH (udpif, list_node, &all_udpifs) { - unsigned int n_offloaded_flows; unsigned int flow_limit; bool ufid_enabled; size_t i; @@ -2919,8 +2916,9 @@ upcall_unixctl_show(struct unixctl_conn *conn, int argc OVS_UNUSED, ds_put_format(&ds, " flows : (current %lu)" " (avg %u) (max %u) (limit %u)\n", udpif_get_n_flows(udpif), udpif->avg_n_flows, udpif->max_n_flows, flow_limit); - atomic_read_relaxed(&udpif->n_offloaded_flows, &n_offloaded_flows); - ds_put_format(&ds, " offloaded flows : %u\n", n_offloaded_flows); + if (!dpif_get_dp_offloaded_flows(udpif->dpif, &n_offloaded_flows)) { + ds_put_format(&ds, " offloaded flows : %ld\n", n_offloaded_flows); + } ds_put_format(&ds, " dump duration : %lldms\n", udpif->dump_duration); ds_put_format(&ds, " ufid enabled : "); if (ufid_enabled) { diff --git a/tests/system-offloads-traffic.at b/tests/system-offloads-traffic.at index 379a8a5e9..c03ffd598 100644 --- a/tests/system-offloads-traffic.at +++ b/tests/system-offloads-traffic.at @@ -32,6 +32,8 @@ in_port(3),eth(macs),eth_type(0x0800),ipv4(frag=no), packets:9, bytes:882, used: AT_CHECK([ovs-appctl dpctl/dump-flows type=offloaded], [0], []) +AT_CHECK([ovs-appctl upcall/show | grep "offloaded flows : 0"], [1], [ignore]) + OVS_TRAFFIC_VSWITCHD_STOP AT_CLEANUP @@ -64,5 +66,7 @@ in_port(2),eth(macs),eth_type(0x0800),ipv4(frag=no), packets:9, bytes:756, used: in_port(3),eth(macs),eth_type(0x0800),ipv4(frag=no), packets:9, bytes:756, used:0.001s, actions:output ]) +AT_CHECK([ovs-appctl upcall/show | grep -E "offloaded flows : [[1-9]]"], [0], [ignore]) + OVS_TRAFFIC_VSWITCHD_STOP AT_CLEANUP
The n_offloaded_flows counter is saved in dpif, and this is the first one when ofproto is created. When flow operation is done by ovs-appctl commands, such as, dpctl/add-flow, a new dpif is opened, and the n_offloaded_flows in it can't be used. So, instead of using counter, the number of offloaded flows is queried from each netdev, then sum them up. To achieve this, a new API is added in netdev_flow_api to get how many flows assigned to a netdev. In order to get better performance, this number is calculated directly from tc_to_ufid hmap for netdev-offload-tc, because flow dumping by tc takes much time if there are many flows offloaded. Fixes: af0618470507 ("dpif-netlink: Count the number of offloaded rules") Signed-off-by: Jianbo Liu <jianbol@nvidia.com> --- lib/dpif-netlink.c | 9 --------- lib/dpif.c | 22 ++++++++++++++++++++++ lib/dpif.h | 3 ++- lib/netdev-offload-provider.h | 4 ++++ lib/netdev-offload-tc.c | 20 ++++++++++++++++++++ lib/netdev-offload.c | 27 +++++++++++++++++++++++++++ lib/netdev-offload.h | 3 +++ ofproto/ofproto-dpif-upcall.c | 10 ++++------ tests/system-offloads-traffic.at | 4 ++++ 9 files changed, 86 insertions(+), 16 deletions(-)