On 1 Feb 2023, at 12:02, Ilya Maximets wrote:
> On 1/31/23 15:38, Eelco Chaudron wrote:
>>
>>
>> On 31 Jan 2023, at 14:13, Ilya Maximets wrote:
>>
>>> On 1/13/23 13:57, Eelco Chaudron wrote:
>>>> When a flow gets modified, i.e. the actions are changes, the tc layer will
>>>> remove, and re-add the flow. This is causing all the counters to be reset.
>>>>
>>>> This patch will remember the previous tc counters and adjust any requests
>>>> for statistics. This is done in a similar way as the rte_flow
>>>> implementation.
>>>>
>>>> It also updates the check_pkt_len tc test to purge the flows, so we do
>>>> not use updated tc flows, with their counters.
>>>
>>> Could you clarify what exactly you mean here?
>>
>> I hope this is more clear:
>>
>> It also updates the check_pkt_len tc test to purge the flows, so we do
>> not use existing updated tc flow counters, but start with fresh installed
>> set of datapath flows.
>>
>>>>
>>>> Signed-off-by: Eelco Chaudron <echau...@redhat.com>
>>>>
>>>> ---
>>>> -v2: Do not update the stats->used, as in terse dump they should be 0.
>>>> -v3: Added some comments based on the v2 review.
>>>>
>>>> Please note that for now two copies of the test case exists, but I will
>>>> clean
>>>> this up once this gets applied by submitting a new revision of the
>>>> 'tests: Add system-traffic.at tests to check-offloads' series.
>>>>
>>>> lib/netdev-offload-tc.c | 98
>>>> ++++++++++++++++++++++++++++++++------
>>>> lib/tc.h | 1
>>>> tests/system-offloads-traffic.at | 65 +++++++++++++++++++++++--
>>>> tests/system-traffic.at | 64 +++++++++++++++++++++++++
>>>> 4 files changed, 207 insertions(+), 21 deletions(-)
>>>>
>>>> diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c
>>>> index ce7f8ad97..59c113187 100644
>>>> --- a/lib/netdev-offload-tc.c
>>>> +++ b/lib/netdev-offload-tc.c
>>>> @@ -97,6 +97,12 @@ static int netdev_tc_parse_nl_actions(struct netdev
>>>> *netdev,
>>>> bool *recirc_act, bool more_actions,
>>>> struct tc_action
>>>> **need_jump_update);
>>>>
>>>> +static void parse_tc_flower_to_stats(struct tc_flower *flower,
>>>> + struct dpif_flow_stats *stats);
>>>> +
>>>> +static int get_ufid_adjust_stats(const ovs_u128 *ufid,
>>>> + struct dpif_flow_stats *stats);
>>>
>>> No need to specify parameter names in prototypes.
>>
>> I know, but all forward declarations in this c file have them included, so
>> I decided to do the same.
>> So I kept them for now, but let me know if you think they should be removed.
>>
>>>> +
>>>> static bool
>>>> is_internal_port(const char *type)
>>>> {
>>>> @@ -193,6 +199,9 @@ static struct ovs_mutex ufid_lock =
>>>> OVS_MUTEX_INITIALIZER;
>>>> * @ufid: ufid assigned to the flow
>>>> * @id: tc filter id (tcf_id)
>>>> * @netdev: netdev associated with the tc rule
>>>> + * @adjust_stats: When flow gets updated with new actions, we need to
>>>> adjust
>>>> + * the reported stats to include previous values as the
>>>> hardware
>>>> + * rule is removed and re-added. This stats copy is used
>>>> for it.
>>>> */
>>>> struct ufid_tc_data {
>>>> struct hmap_node ufid_to_tc_node;
>>>> @@ -200,6 +209,7 @@ struct ufid_tc_data {
>>>> ovs_u128 ufid;
>>>> struct tcf_id id;
>>>> struct netdev *netdev;
>>>> + struct dpif_flow_stats adjust_stats;
>>>> };
>>>>
>>>> static void
>>>> @@ -233,12 +243,37 @@ del_ufid_tc_mapping(const ovs_u128 *ufid)
>>>> ovs_mutex_unlock(&ufid_lock);
>>>> }
>>>>
>>>> +static void
>>>> +netdev_tc_adjust_stats(struct dpif_flow_stats *stats,
>>>> + struct dpif_flow_stats *adjust_stats)
>>>
>>> The adjust_stats pointer could be const.
>>
>> Will do in next rev.
>>
>>>> +{
>>>> + /* Do not try to restore the stats->used, as in terse
>>>> + * mode dumps we will always report them as 0.
>>>
>>> I'm not sure I understand that part. Why we will report them
>>> as zero?
>>
>> If we do a terse dump, we will not process the TLV holding this value, and
>> we will return 0.
>> The revalidator code will override/update this value in udpif_update_used().
>>
>>
>>> Not restoring the 'used' might not be a problem for a general
>>> case, because freshly gathered stats will have more up to date
>>> 'used' value, but that might be a problem for the 'never' case
>>> where updated fow was not used after modification but it was
>>> used before.
>>
>> For the revalidator use case it should always return 0, so it will take care
>> of it with udpif_update_used(). If we return the last used value retriefd
>> during the tc_get_flower() in del_filter_and_ufid_mapping(). It causes the
>> revalidator process to fail, as it will always report the wrong/old value
>> (as it would be larger than 0, which was what I had before).
>
> Hrm, I see.
>
> Maybe re-word the comment to clarify that TC doesn't report
> TCA_ACT_OPTIONS in terse dumps, so the 'lastused' value is
> not available?
>
>>
>>>> + * tcp_flags is not used by tc, so no need to update it. */
>
> s/not used/not collected/ ?
ACK, will update the comment, and send out a v4.
Thanks,
Eelco
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
