[ovs-dev] [PATCH 1/2] netdev-offload: Implement terse dump support
Roi Dayan
roid at mellanox.com
Mon Jun 1 13:01:07 UTC 2020
On 2020-06-01 3:54 PM, Roi Dayan wrote:
> From: Vlad Buslov <vladbu at mellanox.com>
>
> In order to improve revalidator performance by minimizing unnecessary
> copying of data, extend netdev-offloads to support terse dump mode. Extend
> netdev_flow_api->flow_dump_create() with 'terse' bool argument. Implement
> support for terse dump in functions that convert netlink to flower and
> flower to match. Set flow stats "used" value based on difference in number
> of flow packets because lastuse timestamp is not included in TC terse dump.
>
> Kernel API support is implemented in following patch.
>
> Signed-off-by: Vlad Buslov <vladbu at mellanox.com>
> ---
> lib/dpif-netlink.c | 70 ++++++++++++++++++++++---------------------
> lib/netdev-offload-provider.h | 3 +-
> lib/netdev-offload-tc.c | 67 +++++++++++++++++++++++++++++++----------
> lib/netdev-offload.c | 10 ++++---
> lib/netdev-offload.h | 6 ++--
> ofproto/ofproto-dpif-upcall.c | 17 +++++++++--
> 6 files changed, 115 insertions(+), 58 deletions(-)
>
> diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c
> index dc642100fc58..01485c7dc4fa 100644
> --- a/lib/dpif-netlink.c
> +++ b/lib/dpif-netlink.c
> @@ -1445,7 +1445,8 @@ start_netdev_dump(const struct dpif *dpif_,
> dump->netdev_current_dump = 0;
> dump->netdev_dumps
> = netdev_ports_flow_dump_create(dpif_->dpif_class,
> - &dump->netdev_dumps_num);
> + &dump->netdev_dumps_num,
> + dump->up.terse);
> ovs_mutex_unlock(&dump->netdev_lock);
> }
>
> @@ -1640,41 +1641,42 @@ dpif_netlink_netdev_match_to_dpif_flow(struct match *match,
> struct dpif_flow_attrs *attrs,
> ovs_u128 *ufid,
> struct dpif_flow *flow,
> - bool terse OVS_UNUSED)
> -{
> -
> - struct odp_flow_key_parms odp_parms = {
> - .flow = &match->flow,
> - .mask = &match->wc.masks,
> - .support = {
> - .max_vlan_headers = 2,
> - .recirc = true,
> - .ct_state = true,
> - .ct_zone = true,
> - .ct_mark = true,
> - .ct_label = true,
> - },
> - };
> - size_t offset;
> -
> + bool terse)
> +{
> memset(flow, 0, sizeof *flow);
>
> - /* Key */
> - offset = key_buf->size;
> - flow->key = ofpbuf_tail(key_buf);
> - odp_flow_key_from_flow(&odp_parms, key_buf);
> - flow->key_len = key_buf->size - offset;
> -
> - /* Mask */
> - offset = mask_buf->size;
> - flow->mask = ofpbuf_tail(mask_buf);
> - odp_parms.key_buf = key_buf;
> - odp_flow_key_from_mask(&odp_parms, mask_buf);
> - flow->mask_len = mask_buf->size - offset;
> -
> - /* Actions */
> - flow->actions = nl_attr_get(actions);
> - flow->actions_len = nl_attr_get_size(actions);
> + if (!terse) {
> + struct odp_flow_key_parms odp_parms = {
> + .flow = &match->flow,
> + .mask = &match->wc.masks,
> + .support = {
> + .max_vlan_headers = 2,
> + .recirc = true,
> + .ct_state = true,
> + .ct_zone = true,
> + .ct_mark = true,
> + .ct_label = true,
> + },
> + };
> + size_t offset;
> +
> + /* Key */
> + offset = key_buf->size;
> + flow->key = ofpbuf_tail(key_buf);
> + odp_flow_key_from_flow(&odp_parms, key_buf);
> + flow->key_len = key_buf->size - offset;
> +
> + /* Mask */
> + offset = mask_buf->size;
> + flow->mask = ofpbuf_tail(mask_buf);
> + odp_parms.key_buf = key_buf;
> + odp_flow_key_from_mask(&odp_parms, mask_buf);
> + flow->mask_len = mask_buf->size - offset;
> +
> + /* Actions */
> + flow->actions = nl_attr_get(actions);
> + flow->actions_len = nl_attr_get_size(actions);
> + }
>
> /* Stats */
> memcpy(&flow->stats, stats, sizeof *stats);
> diff --git a/lib/netdev-offload-provider.h b/lib/netdev-offload-provider.h
> index 5a809c0cdf1f..0bed7bf61edb 100644
> --- a/lib/netdev-offload-provider.h
> +++ b/lib/netdev-offload-provider.h
> @@ -42,7 +42,8 @@ struct netdev_flow_api {
> *
> * On success returns 0 and allocates data, on failure returns
> * positive errno. */
> - int (*flow_dump_create)(struct netdev *, struct netdev_flow_dump **dump);
> + int (*flow_dump_create)(struct netdev *, struct netdev_flow_dump **dump,
> + bool terse);
> int (*flow_dump_destroy)(struct netdev_flow_dump *);
>
> /* Returns true if there are more flows to dump.
> diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c
> index e188e63e560e..3ba70db4690b 100644
> --- a/lib/netdev-offload-tc.c
> +++ b/lib/netdev-offload-tc.c
> @@ -366,7 +366,8 @@ netdev_tc_flow_flush(struct netdev *netdev)
>
> static int
> netdev_tc_flow_dump_create(struct netdev *netdev,
> - struct netdev_flow_dump **dump_out)
> + struct netdev_flow_dump **dump_out,
> + bool terse)
> {
> enum tc_qdisc_hook hook = get_tc_qdisc_hook(netdev);
> struct netdev_flow_dump *dump;
> @@ -386,6 +387,7 @@ netdev_tc_flow_dump_create(struct netdev *netdev,
> dump = xzalloc(sizeof *dump);
> dump->nl_dump = xzalloc(sizeof *dump->nl_dump);
> dump->netdev = netdev_ref(netdev);
> + dump->terse = terse;
>
> id = tc_make_tcf_id(ifindex, block_id, prio, hook);
> tc_dump_flower_start(&id, dump->nl_dump);
> @@ -502,13 +504,53 @@ flower_tun_opt_to_match(struct match *match, struct tc_flower *flower)
> match->wc.masks.tunnel.flags |= FLOW_TNL_F_UDPIF;
> }
>
> +static void
> +parse_tc_flower_to_stats(struct tc_flower *flower,
> + struct dpif_flow_stats *stats)
> +{
> + if (!stats) {
> + return;
> + }
> +
> + memset(stats, 0, sizeof *stats);
> + stats->n_packets = get_32aligned_u64(&flower->stats.n_packets);
> + stats->n_bytes = get_32aligned_u64(&flower->stats.n_bytes);
> + stats->used = flower->lastused;
> +}
> +
> +static void
> +parse_tc_flower_to_attrs(struct tc_flower *flower,
> + struct dpif_flow_attrs *attrs)
> +{
> + attrs->offloaded = (flower->offloaded_state == TC_OFFLOADED_STATE_IN_HW ||
> + flower->offloaded_state ==
> + TC_OFFLOADED_STATE_UNDEFINED);
> + attrs->dp_layer = "tc";
> + attrs->dp_extra_info = NULL;
> +}
> +
> +static int
> +parse_tc_flower_terse_to_match(struct tc_flower *flower,
> + struct match *match,
> + struct dpif_flow_stats *stats,
> + struct dpif_flow_attrs *attrs)
> +{
> + match_init_catchall(match);
> +
> + parse_tc_flower_to_stats(flower, stats);
> + parse_tc_flower_to_attrs(flower, attrs);
> +
> + return 0;
> +}
> +
> static int
> parse_tc_flower_to_match(struct tc_flower *flower,
> struct match *match,
> struct nlattr **actions,
> struct dpif_flow_stats *stats,
> struct dpif_flow_attrs *attrs,
> - struct ofpbuf *buf)
> + struct ofpbuf *buf,
> + bool terse)
> {
> size_t act_off;
> struct tc_flower_key *key = &flower->key;
> @@ -517,6 +559,10 @@ parse_tc_flower_to_match(struct tc_flower *flower,
> struct tc_action *action;
> int i;
>
> + if (terse) {
> + return parse_tc_flower_terse_to_match(flower, match, stats, attrs);
> + }
> +
> ofpbuf_clear(buf);
>
> match_init_catchall(match);
> @@ -863,17 +909,8 @@ parse_tc_flower_to_match(struct tc_flower *flower,
>
> *actions = ofpbuf_at_assert(buf, act_off, sizeof(struct nlattr));
>
> - if (stats) {
> - memset(stats, 0, sizeof *stats);
> - stats->n_packets = get_32aligned_u64(&flower->stats.n_packets);
> - stats->n_bytes = get_32aligned_u64(&flower->stats.n_bytes);
> - stats->used = flower->lastused;
> - }
> -
> - attrs->offloaded = (flower->offloaded_state == TC_OFFLOADED_STATE_IN_HW)
> - || (flower->offloaded_state == TC_OFFLOADED_STATE_UNDEFINED);
> - attrs->dp_layer = "tc";
> - attrs->dp_extra_info = NULL;
> + parse_tc_flower_to_stats(flower, stats);
> + parse_tc_flower_to_attrs(flower, attrs);
>
> return 0;
> }
> @@ -905,7 +942,7 @@ netdev_tc_flow_dump_next(struct netdev_flow_dump *dump,
> }
>
> if (parse_tc_flower_to_match(&flower, match, actions, stats, attrs,
> - wbuffer)) {
> + wbuffer, dump->terse)) {
> continue;
> }
>
> @@ -1784,7 +1821,7 @@ netdev_tc_flow_get(struct netdev *netdev,
> }
>
> in_port = netdev_ifindex_to_odp_port(id.ifindex);
> - parse_tc_flower_to_match(&flower, match, actions, stats, attrs, buf);
> + parse_tc_flower_to_match(&flower, match, actions, stats, attrs, buf, false);
>
> match->wc.masks.in_port.odp_port = u32_to_odp(UINT32_MAX);
> match->flow.in_port.odp_port = in_port;
> diff --git a/lib/netdev-offload.c b/lib/netdev-offload.c
> index 32eab5910760..ab97a292ebac 100644
> --- a/lib/netdev-offload.c
> +++ b/lib/netdev-offload.c
> @@ -201,13 +201,14 @@ netdev_flow_flush(struct netdev *netdev)
> }
>
> int
> -netdev_flow_dump_create(struct netdev *netdev, struct netdev_flow_dump **dump)
> +netdev_flow_dump_create(struct netdev *netdev, struct netdev_flow_dump **dump,
> + bool terse)
> {
> const struct netdev_flow_api *flow_api =
> ovsrcu_get(const struct netdev_flow_api *, &netdev->flow_api);
>
> return (flow_api && flow_api->flow_dump_create)
> - ? flow_api->flow_dump_create(netdev, dump)
> + ? flow_api->flow_dump_create(netdev, dump, terse)
> : EOPNOTSUPP;
> }
>
> @@ -436,7 +437,8 @@ netdev_ports_flow_flush(const struct dpif_class *dpif_class)
> }
>
> struct netdev_flow_dump **
> -netdev_ports_flow_dump_create(const struct dpif_class *dpif_class, int *ports)
> +netdev_ports_flow_dump_create(const struct dpif_class *dpif_class, int *ports,
> + bool terse)
> {
> struct port_to_netdev_data *data;
> struct netdev_flow_dump **dumps;
> @@ -454,7 +456,7 @@ netdev_ports_flow_dump_create(const struct dpif_class *dpif_class, int *ports)
>
> HMAP_FOR_EACH (data, portno_node, &port_to_netdev) {
> if (data->dpif_class == dpif_class) {
> - if (netdev_flow_dump_create(data->netdev, &dumps[i])) {
> + if (netdev_flow_dump_create(data->netdev, &dumps[i], terse)) {
> continue;
> }
>
> diff --git a/lib/netdev-offload.h b/lib/netdev-offload.h
> index b4b882a56aac..87f5852c8d60 100644
> --- a/lib/netdev-offload.h
> +++ b/lib/netdev-offload.h
> @@ -80,7 +80,8 @@ struct offload_info {
> };
>
> int netdev_flow_flush(struct netdev *);
> -int netdev_flow_dump_create(struct netdev *, struct netdev_flow_dump **dump);
> +int netdev_flow_dump_create(struct netdev *, struct netdev_flow_dump **dump,
> + bool terse);
> int netdev_flow_dump_destroy(struct netdev_flow_dump *);
> bool netdev_flow_dump_next(struct netdev_flow_dump *, struct match *,
> struct nlattr **actions, struct dpif_flow_stats *,
> @@ -114,7 +115,8 @@ odp_port_t netdev_ifindex_to_odp_port(int ifindex);
>
> struct netdev_flow_dump **netdev_ports_flow_dump_create(
> const struct dpif_class *,
> - int *ports);
> + int *ports,
> + bool terse);
> void netdev_ports_flow_flush(const struct dpif_class *);
> int netdev_ports_flow_del(const struct dpif_class *, const ovs_u128 *ufid,
> struct dpif_flow_stats *stats);
> diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
> index 5e08ef10dad6..a5ff3a336ff1 100644
> --- a/ofproto/ofproto-dpif-upcall.c
> +++ b/ofproto/ofproto-dpif-upcall.c
> @@ -2576,6 +2576,18 @@ udpif_update_flow_pps(struct udpif *udpif, struct udpif_key *ukey,
> ukey->flow_time = udpif->dpif->current_ms;
> }
>
> +static long long int
> +udpif_update_used(struct udpif *udpif, struct udpif_key *ukey,
> + struct dpif_flow_stats *stats)
> +{
> + if (udpif->dump->terse) {
> + stats->used = stats->n_packets > ukey->stats.n_packets ?
> + udpif->dpif->current_ms : ukey->stats.used;
> + return stats->used;
> + }
> + return ukey->created;
> +}
> +
> static void
> revalidate(struct revalidator *revalidator)
> {
> @@ -2631,6 +2643,7 @@ revalidate(struct revalidator *revalidator)
> for (f = flows; f < &flows[n_dumped]; f++) {
> long long int used = f->stats.used;
> struct recirc_refs recircs = RECIRC_REFS_EMPTY_INITIALIZER;
> + struct dpif_flow_stats stats = f->stats;
> enum reval_result result;
> struct udpif_key *ukey;
> bool already_dumped;
> @@ -2675,12 +2688,12 @@ revalidate(struct revalidator *revalidator)
> }
>
> if (!used) {
> - used = ukey->created;
> + used = udpif_update_used(udpif, ukey, &stats);
> }
> if (kill_them_all || (used && used < now - max_idle)) {
> result = UKEY_DELETE;
> } else {
> - result = revalidate_ukey(udpif, ukey, &f->stats, &odp_actions,
> + result = revalidate_ukey(udpif, ukey, &stats, &odp_actions,
> reval_seq, &recircs,
> f->attrs.offloaded);
> }
>
Reviewed-by: Roi Dayan <roid at mellanox.com>
More information about the dev
mailing list