> -----Original Message-----
> From: Ilya Maximets <i.maxim...@samsung.com>
> Sent: Tuesday, March 12, 2019 5:25 PM
> To: Roni Bar Yanai <ron...@mellanox.com>; 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>
> Subject: Re: [PATCH v1] dpif: Add marked packets stats
>
> On 12.03.2019 17:53, Roni Bar Yanai wrote:
> >
> >
> >> -----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.
>
> Not sure if I understand your point correctly, but isn't it expected that
> if flow is offloaded than all the packets through the flow had flow mark?
> In case of a bug it's most likely that it happened after the flow mark
> association and your flow stats will be wrong anyway.
> The only trusted source of the number of "offloaded" packets could be the
> number of real packets that had flow mark set on receive. And these stats
> should be displayed by PMD stats.
>
> If everything works as expected, all the packets for offloaded flow will be
> marked (except first few of them).
> If something will go wrong, we can't trust these flow stats. We can rely only
Why you can't trust the flow stats? This is the only point in the code where
you know that the flow was really handled by hardware.
> on PMD stats.
> So, I don't see the profit from having these special flow stats.
>
I'm looking at it from the OVS user point of view. I have thousands of flows
running.
If the statistics is only in the PMD I can see how many packets were marked and
how many
we're not. Let's say the ratio is not that good, how do I look deeper?
Is it because for some flows there is bug on matching? Is it because of the
latency in adding flows?
Is it because some flow pattern is not supported at all?
Agree this is only needed for debugging, but I don't see other way user can get
this 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
