[ovs-dev] [PATCH v1] dpif: Add marked packets stats
Ilya Maximets
i.maximets at samsung.com
Tue Mar 12 15:25:29 UTC 2019
On 12.03.2019 17:53, Roni Bar Yanai wrote:
>
>
>> -----Original Message-----
>> From: Ilya Maximets <i.maximets at samsung.com>
>> Sent: Tuesday, March 12, 2019 11:11 AM
>> To: Ophir Munk <ophirmu at mellanox.com>; ovs-dev at openvswitch.org
>> Cc: Asaf Penso <asafp at mellanox.com>; Ian Stokes <ian.stokes at intel.com>;
>> Shahaf Shuler <shahafs at mellanox.com>; Olga Shern
>> <olgas at mellanox.com>; Kevin Traynor <ktraynor at redhat.com>; Roni Bar
>> Yanai <roniba at 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 <ophirmu at 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
on PMD stats.
So, I don't see the profit from having these special flow stats.
>
> 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);
>>>
More information about the dev
mailing list