On Mon, 29 Nov 2021, Ilya Maximets wrote:
> On 11/7/21 13:12, Roi Dayan via dev wrote:
> > From: Paul Blakey <[email protected]>
> >
> > Fragmented packets with offset=0 are defragmented by tc act_ct, and
> > only when assembled pass to next action, in ovs offload case,
> > a goto action. Since stats are overwritten on each action dump,
> > only the stats for last action in the tc filter action priority
> > list is taken, the stats on the goto action, which count
> > only the assembled packets. See below for example.
> >
> > Hardware updates just part of the actions (gact, ct, mirred) - those
> > that support stats_update() operation. Since datapath rules end
> > with either an output (mirred) or recirc/drop (both gact), tc rule
> > will at least have one action that supports it. For software packets,
> > the first action will have the max software packets count.
> > Tc dumps total packets (hw + sw) and hardware packets, then
> > software packets needs to be calculated from this (total - hw).
> >
> > To fix the above, get hardware packets and calculate software packets
> > for each action, take the max of each set, then combine back
> > to get the total packets that went through software and hardware.
> >
> > Example by running ping above MTU (ping <IP> -s 2000):
> > ct_state(-trk),recirc_id(0),...,ipv4(proto=1,frag=first),
> > packets:14, bytes:19544,..., actions:ct(zone=1),recirc(0x1)
> > ct_state(-trk),recirc_id(0),...,ipv4(proto=1,frag=later),
> > packets:14, bytes:28392,..., actions:ct(zone=1),recirc(0x1)
> >
> > Second rule should have had bytes=14*<size of 'later' frag>, but instead
> > it's bytes=14*<size of assembled packets - size of 'first' + 'later'
> > frags>.
> >
> > Fixes: 576126a931cd ("netdev-offload-tc: Add conntrack support")
> > Signed-off-by: Paul Blakey <[email protected]>
> > Reviewed-by: Roi Dayan <[email protected]>
> > ---
> > lib/netdev-offload-tc.c | 10 +++++-----
> > lib/tc.c | 34 ++++++++++++++++++++++++++++------
> > lib/tc.h | 3 ++-
> > 3 files changed, 35 insertions(+), 12 deletions(-)
> >
> > diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c
> > index 9845e8d3feae..3f7068c8e066 100644
> > --- a/lib/netdev-offload-tc.c
> > +++ b/lib/netdev-offload-tc.c
> > @@ -585,8 +585,10 @@ parse_tc_flower_to_stats(struct tc_flower *flower,
> > }
> >
> > 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->n_packets = get_32aligned_u64(&flower->stats_sw.n_packets);
> > + stats->n_packets += get_32aligned_u64(&flower->stats_hw.n_packets);
> > + stats->n_bytes = get_32aligned_u64(&flower->stats_sw.n_bytes);
> > + stats->n_bytes += get_32aligned_u64(&flower->stats_hw.n_bytes);
> > stats->used = flower->lastused;
> > }
> >
> > @@ -2015,9 +2017,7 @@ netdev_tc_flow_del(struct netdev *netdev OVS_UNUSED,
> > if (stats) {
> > memset(stats, 0, sizeof *stats);
> > if (!tc_get_flower(&id, &flower)) {
> > - stats->n_packets = get_32aligned_u64(&flower.stats.n_packets);
> > - stats->n_bytes = get_32aligned_u64(&flower.stats.n_bytes);
> > - stats->used = flower.lastused;
> > + parse_tc_flower_to_stats(&flower, stats);
> > }
> > }
> >
> > diff --git a/lib/tc.c b/lib/tc.c
> > index 38a1dfc0ebc8..961aef619815 100644
> > --- a/lib/tc.c
> > +++ b/lib/tc.c
> > @@ -1702,6 +1702,9 @@ static const struct nl_policy stats_policy[] = {
> > [TCA_STATS_BASIC] = { .type = NL_A_UNSPEC,
> > .min_len = sizeof(struct gnet_stats_basic),
> > .optional = false, },
> > + [TCA_STATS_BASIC_HW] = { .type = NL_A_UNSPEC,
> > + .min_len = sizeof(struct gnet_stats_basic),
> > + .optional = true, },
> > };
> >
> > static int
> > @@ -1714,8 +1717,11 @@ nl_parse_single_action(struct nlattr *action, struct
> > tc_flower *flower,
> > const char *act_kind;
> > struct nlattr *action_attrs[ARRAY_SIZE(act_policy)];
> > struct nlattr *stats_attrs[ARRAY_SIZE(stats_policy)];
> > - struct ovs_flow_stats *stats = &flower->stats;
> > - const struct gnet_stats_basic *bs;
> > + struct ovs_flow_stats *stats_sw = &flower->stats_sw;
> > + struct ovs_flow_stats *stats_hw = &flower->stats_hw;
> > + const struct gnet_stats_basic *bs_all = NULL;
> > + const struct gnet_stats_basic *bs_hw = NULL;
> > + struct gnet_stats_basic bs_sw = { .packets = 0, .bytes = 0, };
> > int err = 0;
> >
> > if (!nl_parse_nested(action, act_policy, action_attrs,
> > @@ -1771,10 +1777,26 @@ nl_parse_single_action(struct nlattr *action,
> > struct tc_flower *flower,
> > return EPROTO;
> > }
> >
> > - bs = nl_attr_get_unspec(stats_attrs[TCA_STATS_BASIC], sizeof *bs);
> > - if (bs->packets) {
> > - put_32aligned_u64(&stats->n_packets, bs->packets);
> > - put_32aligned_u64(&stats->n_bytes, bs->bytes);
> > + bs_all = nl_attr_get_unspec(stats_attrs[TCA_STATS_BASIC], sizeof
> > *bs_all);
> > + if (stats_attrs[TCA_STATS_BASIC_HW]) {
> > + bs_hw = nl_attr_get_unspec(stats_attrs[TCA_STATS_BASIC_HW],
> > + sizeof *bs_hw);
> > +
> > + bs_sw.packets = bs_all->packets - bs_hw->packets;
> > + bs_sw.bytes = bs_all->bytes - bs_hw->bytes;
>
> IIUC, there are kernels that supports some kind of TC offload, but
> has no support for TCA_STATS_BASIC_HW. If that right?
> In this case, I suppose, bs_hw will be NULL here and OVS will crash.
>
> Best regards, Ilya Maximets.
>
Yes, but for such cases they will fail the check for "if
(stats_attrs[TCA_STATS_BASIC_HW]) {" and not access bs_hw.
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
