| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Mon, 1 Nov 2021 10:07:50 +0000
From: Baowen Zheng <baowen.zheng@...igine.com>
To: Vlad Buslov <vladbu@...dia.com>,
Simon Horman <simon.horman@...igine.com>
CC: "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
Jamal Hadi Salim <jhs@...atatu.com>,
Roi Dayan <roid@...dia.com>, Ido Schimmel <idosch@...dia.com>,
Cong Wang <xiyou.wangcong@...il.com>,
Jiri Pirko <jiri@...nulli.us>,
Baowen Zheng <notifications@...hub.com>,
Louis Peens <louis.peens@...igine.com>,
oss-drivers <oss-drivers@...igine.com>
Subject: RE: [RFC/PATCH net-next v3 5/8] flow_offload: add process to update
action stats from hardware
On October 30, 2021 1:11 AM, Vlad Buslov <vladbu@...dia.com> wrote:
>On Thu 28 Oct 2021 at 14:06, Simon Horman <simon.horman@...igine.com>
>wrote:
>> From: Baowen Zheng <baowen.zheng@...igine.com>
>>
>> When collecting stats for actions update them using both both hardware
>> and software counters.
>>
>> Stats update process should not in context of preempt_disable.
>
>I think you are missing a word here.
Thanks, we will fix it in next patch.
>>
>> Signed-off-by: Baowen Zheng <baowen.zheng@...igine.com>
>> Signed-off-by: Louis Peens <louis.peens@...igine.com>
>> Signed-off-by: Simon Horman <simon.horman@...igine.com>
>> ---
>> include/net/act_api.h | 1 +
>> include/net/pkt_cls.h | 18 ++++++++++--------
>> net/sched/act_api.c | 37 +++++++++++++++++++++++++++++++++++++
>> 3 files changed, 48 insertions(+), 8 deletions(-)
>>
>> diff --git a/include/net/act_api.h b/include/net/act_api.h index
>> 671208bd27ef..80a9d1e7d805 100644
>> --- a/include/net/act_api.h
>> +++ b/include/net/act_api.h
>> @@ -247,6 +247,7 @@ void tcf_action_update_stats(struct tc_action *a,
>u64 bytes, u64 packets,
>> u64 drops, bool hw);
>> int tcf_action_copy_stats(struct sk_buff *, struct tc_action *, int);
>> int tcf_action_offload_del(struct tc_action *action);
>> +int tcf_action_update_hw_stats(struct tc_action *action);
>> int tcf_action_check_ctrlact(int action, struct tcf_proto *tp,
>> struct tcf_chain **handle,
>> struct netlink_ext_ack *newchain); diff --git
>> a/include/net/pkt_cls.h b/include/net/pkt_cls.h index
>> 44ae5182a965..88788b821f76 100644
>> --- a/include/net/pkt_cls.h
>> +++ b/include/net/pkt_cls.h
>> @@ -292,18 +292,20 @@ tcf_exts_stats_update(const struct tcf_exts
>> *exts, #ifdef CONFIG_NET_CLS_ACT
>> int i;
>>
>> - preempt_disable();
>> -
>> for (i = 0; i < exts->nr_actions; i++) {
>> struct tc_action *a = exts->actions[i];
>>
>> - tcf_action_stats_update(a, bytes, packets, drops,
>> - lastuse, true);
>> - a->used_hw_stats = used_hw_stats;
>> - a->used_hw_stats_valid = used_hw_stats_valid;
>> - }
>> + /* if stats from hw, just skip */
>> + if (tcf_action_update_hw_stats(a)) {
>> + preempt_disable();
>> + tcf_action_stats_update(a, bytes, packets, drops,
>> + lastuse, true);
>> + preempt_enable();
>>
>> - preempt_enable();
>> + a->used_hw_stats = used_hw_stats;
>> + a->used_hw_stats_valid = used_hw_stats_valid;
>> + }
>> + }
>> #endif
>> }
>>
>> diff --git a/net/sched/act_api.c b/net/sched/act_api.c index
>> 604bf1923bcc..881c7ba4d180 100644
>> --- a/net/sched/act_api.c
>> +++ b/net/sched/act_api.c
>> @@ -1238,6 +1238,40 @@ static int tcf_action_offload_add(struct tc_action
>*action,
>> return err;
>> }
>>
>> +int tcf_action_update_hw_stats(struct tc_action *action) {
>> + struct flow_offload_action fl_act = {};
>> + int err = 0;
>> +
>> + if (!tc_act_in_hw(action))
>> + return -EOPNOTSUPP;
>> +
>> + err = flow_action_init(&fl_act, action, FLOW_ACT_STATS, NULL);
>> + if (err)
>> + goto err_out;
>> +
>> + err = tcf_action_offload_cmd(&fl_act, NULL, NULL);
>> +
>> + if (!err && fl_act.stats.lastused) {
>> + preempt_disable();
>> + tcf_action_stats_update(action, fl_act.stats.bytes,
>> + fl_act.stats.pkts,
>> + fl_act.stats.drops,
>> + fl_act.stats.lastused,
>> + true);
>> + preempt_enable();
>> + action->used_hw_stats = fl_act.stats.used_hw_stats;
>> + action->used_hw_stats_valid = true;
>> + err = 0;
>
>Error handling here is slightly convoluted. This line assigns err=0 third time (it is
>initialized with zero and then we can only get here if result of
>tcf_action_offload_cmd() assigned 'err' to zero again).
>Considering that error handler in this function is empty we can just return
>errors directly as soon as they happen and return zero at the end of the
>function.
>
Thanks, we will change as your suggestion.
>> + } else {
>> + err = -EOPNOTSUPP;
>
>Hmm the code can return error here when tcf_action_offload_cmd()
>succeeded but 'lastused' is zero. Such behavior will cause
>tcf_exts_stats_update() to update action with filter counter values. Is this the
>desired behavior when, for example, in filter action list there is and action that
>can drop packets followed by some shared action? In such case 'lastused' can
>be zero if all packets that filter matched were dropped by previous action and
>shared action will be assigned with filter counter value that includes dropped
>packets/bytes.
>
Thanks, we will consider if it make sense to only judge return value err from tcf_action_offload_cmd.
>> + }
>> +
>> +err_out:
>> + return err;
>> +}
>> +EXPORT_SYMBOL(tcf_action_update_hw_stats);
>> +
>> int tcf_action_offload_del(struct tc_action *action) {
>> struct flow_offload_action fl_act;
>> @@ -1362,6 +1396,9 @@ int tcf_action_copy_stats(struct sk_buff *skb,
>struct tc_action *p,
>> if (p == NULL)
>> goto errout;
>>
>> + /* update hw stats for this action */
>> + tcf_action_update_hw_stats(p);
>> +
>> /* compat_mode being true specifies a call that is supposed
>> * to add additional backward compatibility statistic TLVs.
>> */
Powered by blists - more mailing lists