> -----Original Message-----
> From: Ilya Maximets <i.maxim...@samsung.com>
> Sent: Tuesday, March 12, 2019 11:11 AM
> To: Ophir Munk <ophi...@mellanox.com>; ovs-dev@openvswitch.org
> Cc: Asaf Penso <as...@mellanox.com>; Ian Stokes <ian.sto...@intel.com>;
> Shahaf Shuler <shah...@mellanox.com>; Olga Shern
> <ol...@mellanox.com>; Kevin Traynor <ktray...@redhat.com>; Roni Bar
> Yanai <ron...@mellanox.com>
> Subject: Re: [PATCH v1] dpif: Add marked packets stats
>
> On 12.03.2019 11:41, Ophir Munk wrote:
> > This commit adds marked packets statistics. Following commit [1],
> > received packets can be marked with a mark id which is associated
> > with the flow on which the packets were recieved. This commits counts
> > the marked packets per flow and displays the result when executing
> > datapath dump-flow command with a "-m" (more verbosity) option, as
> > shown in [2].
> > Packets are marked only when hw-offload option is enabled and only on
> > some netdev classes such as netdev-dpdk. In all other cases marked
> > packets are not counted and are not displayed when executing the
> command
> > in [2].
> >
> > [1]
> > Commit 241bad15d9 ("dpif-netdev: associate flow with a mark id")
> >
> > [2]
> > ovs-appctl dpif/dump-flows -m <bridge-name>
> >
> > Signed-off-by: Ophir Munk <ophi...@mellanox.com>
> > ---
>
> Hi Ophir.
>
> Thanks for working on this, but I don't think that flow stats is the right
> place to expose information like that. We already have pmd-stats/perf-show
> appctl calls that prints various hit stats for PMD threads. And, I think,
> 'flow mark hits' or something similar should be one of these stats.
> Right now flow mark hits are not accounted there and that is the issue.
>
> Regarding the flow stats, we probably need to expose status of the flow,
> i.e. if it was offloaded or not, like it done for tc offloading.
> Right now all the flows reported by dpif-netdev as not offloaded.
> See 'flow->attrs.offloaded = false;' in dp_netdev_flow_to_dpif_flow().
> And we probably need to change this from boolean to some 'yes/no/partial'.
>
> BTW, your current implementation assumes that packets had flow mark if
> 'flow->mark' is set, but this is could be not true, because packets could
> arrive while offloading is in progress or offloading could be started
> between packets arrival and finishing of their processing.
>
> Best regards, Ilya Maximets.
Hi Ilya,
We agree that flow should be marked as offloaded yes/no/partial, but seems this
is not enough.
Like you said, since hardware offload is executed in a different thread, it
might be that the number
of packets that were marked/handled by hardware is different than the total
packets of the flow.
If there is a load on the offload or just high latency, it might be that in
some cases the difference will be significant.
Although I agree that you will probably be able to see the problem In the
pmd-stats (assuming mark will be added there),
,it will be less clear. For example, there are probably flows that cannot be
offloaded at all so you won't expect
mark count for them. Another example is a bug, we say that the flow was
offloaded but in fact it is not.
Of course you should expect the code to be tested, but the field is always
different.
Maybe we can add it we special flag for hardware offload specific information.
Thanks
>
> > lib/dpif-netdev.c | 16 ++++++++++++++++
> > lib/dpif.c | 9 +++++++++
> > lib/dpif.h | 3 +++
> > ofproto/ofproto-dpif.c | 4 ++++
> > 4 files changed, 32 insertions(+)
> >
> > diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> > index 4d6d0c3..ef2f8f1 100644
> > --- a/lib/dpif-netdev.c
> > +++ b/lib/dpif-netdev.c
> > @@ -486,6 +486,8 @@ struct dp_netdev_flow_stats {
> > atomic_ullong packet_count; /* Number of packets matched. */
> > atomic_ullong byte_count; /* Number of bytes matched. */
> > atomic_uint16_t tcp_flags; /* Bitwise-OR of seen tcp_flags values.
> > */
> > + /* Offload stats. */
> > + atomic_ullong mark_count; /* Number of marked packets matched.
> */
> > };
> >
> > /* A flow in 'dp_netdev_pmd_thread's 'flow_table'.
> > @@ -3008,6 +3010,9 @@ get_dpif_flow_stats(const struct
> dp_netdev_flow *netdev_flow_,
> > stats->used = used;
> > atomic_read_relaxed(&netdev_flow->stats.tcp_flags, &flags);
> > stats->tcp_flags = flags;
> > + /* Offload stats. */
> > + atomic_read_relaxed(&netdev_flow->stats.mark_count, &n);
> > + stats->n_marked = n;
> > }
> >
> > /* Converts to the dpif_flow format, using 'key_buf' and 'mask_buf' for
> > @@ -6124,6 +6129,15 @@ dp_netdev_flow_used(struct dp_netdev_flow
> *netdev_flow, int cnt, int size,
> > atomic_store_relaxed(&netdev_flow->stats.tcp_flags, flags);
> > }
> >
> > +static void
> > +dp_netdev_flow_marked(struct dp_netdev_flow *netdev_flow,
> uint32_t mark,
> > + int count)
> > +{
> > + if (mark != INVALID_FLOW_MARK) {
> > + non_atomic_ullong_add(&netdev_flow->stats.mark_count, count);
> > + }
> > +}
> > +
> > static int
> > dp_netdev_upcall(struct dp_netdev_pmd_thread *pmd, struct dp_packet
> *packet_,
> > struct flow *flow, struct flow_wildcards *wc, ovs_u128
> > *ufid,
> > @@ -6244,6 +6258,8 @@ packet_batch_per_flow_execute(struct
> packet_batch_per_flow *batch,
> > dp_netdev_flow_used(flow, batch->array.count, batch->byte_count,
> > batch->tcp_flags, pmd->ctx.now / 1000);
> >
> > + dp_netdev_flow_marked(flow, batch->flow->mark, batch-
> >array.count);
> > +
> > actions = dp_netdev_flow_get_actions(flow);
> >
> > dp_netdev_execute_actions(pmd, &batch->array, true, &flow->flow,
> > diff --git a/lib/dpif.c b/lib/dpif.c
> > index 457c9bf..bb077a5 100644
> > --- a/lib/dpif.c
> > +++ b/lib/dpif.c
> > @@ -880,9 +880,18 @@ dpif_flow_stats_extract(const struct flow *flow,
> const struct dp_packet *packet,
> > stats->tcp_flags = ntohs(flow->tcp_flags);
> > stats->n_bytes = dp_packet_size(packet);
> > stats->n_packets = 1;
> > + stats->n_marked = 0;
> > stats->used = used;
> > }
> >
> > +void
> > +dpif_offload_stats_format(const struct dpif_flow_stats *stats, struct ds
> *s)
> > +{
> > + if (stats->n_marked) {
> > + ds_put_format(s, "marked:%"PRIu64", ", stats->n_marked);
> > + }
> > +}
> > +
> > /* Appends a human-readable representation of 'stats' to 's'. */
> > void
> > dpif_flow_stats_format(const struct dpif_flow_stats *stats, struct ds *s)
> > diff --git a/lib/dpif.h b/lib/dpif.h
> > index 475d5a6..7c22afd 100644
> > --- a/lib/dpif.h
> > +++ b/lib/dpif.h
> > @@ -492,6 +492,8 @@ struct dpif_flow_stats {
> > uint64_t n_bytes;
> > long long int used;
> > uint16_t tcp_flags;
> > + /* Offload stats. */
> > + uint64_t n_marked;
> > };
> >
> > struct dpif_flow_attrs {
> > @@ -507,6 +509,7 @@ struct dpif_flow_dump_types {
> > void dpif_flow_stats_extract(const struct flow *, const struct dp_packet
> *packet,
> > long long int used, struct dpif_flow_stats *);
> > void dpif_flow_stats_format(const struct dpif_flow_stats *, struct ds *);
> > +void dpif_offload_stats_format(const struct dpif_flow_stats *, struct ds
> *);
> >
> > enum dpif_flow_put_flags {
> > DPIF_FP_CREATE = 1 << 0, /* Allow creating a new flow. */
> > diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
> > index 21dd54b..af37ecb 100644
> > --- a/ofproto/ofproto-dpif.c
> > +++ b/ofproto/ofproto-dpif.c
> > @@ -4416,6 +4416,7 @@ rule_construct(struct rule *rule_)
> > rule->stats.n_packets = 0;
> > rule->stats.n_bytes = 0;
> > rule->stats.used = rule->up.modified;
> > + rule->stats.n_marked = 0;
> > rule->recirc_id = 0;
> > rule->new_rule = NULL;
> > rule->forward_counts = false;
> > @@ -5709,6 +5710,9 @@ ofproto_unixctl_dpif_dump_flows(struct
> unixctl_conn *conn,
> > odp_flow_format(f.key, f.key_len, f.mask, f.mask_len,
> > portno_names, &ds, verbosity);
> > ds_put_cstr(&ds, ", ");
> > + if (verbosity) {
> > + dpif_offload_stats_format(&f.stats, &ds);
> > + }
> > dpif_flow_stats_format(&f.stats, &ds);
> > ds_put_cstr(&ds, ", actions:");
> > format_odp_actions(&ds, f.actions, f.actions_len, portno_names);
> >
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
