| 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
| ||
|
Message-ID: <Y9UemKFXoekIISvC@corigine.com>
Date: Sat, 28 Jan 2023 14:09:44 +0100
From: Simon Horman <simon.horman@...igine.com>
To: Pedro Tammela <pctammela@...atatu.com>
Cc: netdev@...r.kernel.org, jhs@...atatu.com, xiyou.wangcong@...il.com,
jiri@...nulli.us, davem@...emloft.net, edumazet@...gle.com,
kuba@...nel.org, pabeni@...hat.com
Subject: Re: [PATCH net-next v3] net/sched: transition act_pedit to rcu and
percpu stats
On Fri, Jan 27, 2023 at 04:27:52PM -0300, Pedro Tammela wrote:
> The software pedit action didn't get the same love as some of the
> other actions and it's still using spinlocks and shared stats.
> Transition the action to rcu and percpu stats which improves the
> action's performance dramatically.
>
> We test this change with a very simple packet forwarding setup:
>
> tc filter add dev ens2f0 ingress protocol ip matchall \
> action pedit ex munge eth src set b8:ce:f6:4b:68:35 pipe \
> action pedit ex munge eth dst set ac:1f:6b:e4:ff:93 pipe \
> action mirred egress redirect dev ens2f1
> tc filter add dev ens2f1 ingress protocol ip matchall \
> action pedit ex munge eth src set b8:ce:f6:4b:68:34 pipe \
> action pedit ex munge eth dst set ac:1f:6b:e4:ff:92 pipe \
> action mirred egress redirect dev ens2f0
>
> Using TRex with a http-like profile, in our setup with a 25G NIC
> and a 26 cores Intel CPU, we observe the following in perf:
> before:
> 11.59% 2.30% [kernel] [k] tcf_pedit_act
> 2.55% tcf_pedit_act
> 8.38% _raw_spin_lock
> 6.43% native_queued_spin_lock_slowpath
> after:
> 1.46% 1.46% [kernel] [k] tcf_pedit_act
>
> Signed-off-by: Pedro Tammela <pctammela@...atatu.com>
> Reviewed-by: Jamal Hadi Salim <jhs@...atatu.com>
Hi Pedro,
in general this patch looks good to me.
In the course of reviewing it I cam up with a few comments which I have
provided below. Please consider these as being for informational purposes.
>From my perspective they don't warrant a respin.
> diff --git a/include/net/tc_act/tc_pedit.h b/include/net/tc_act/tc_pedit.h
> index 3e02709a1df6..7c597db2b4fa 100644
> --- a/include/net/tc_act/tc_pedit.h
> +++ b/include/net/tc_act/tc_pedit.h
...
> @@ -32,37 +39,81 @@ static inline bool is_tcf_pedit(const struct tc_action *a)
>
> static inline int tcf_pedit_nkeys(const struct tc_action *a)
> {
> - return to_pedit(a)->tcfp_nkeys;
> + struct tcf_pedit_parms *parms;
> + int nkeys;
> +
> + rcu_read_lock();
> + parms = to_pedit_parms(a);
> + nkeys = parms->tcfp_nkeys;
> + rcu_read_unlock();
> +
> + return nkeys;
> }
>
> static inline u32 tcf_pedit_htype(const struct tc_action *a, int index)
> {
> - if (to_pedit(a)->tcfp_keys_ex)
> - return to_pedit(a)->tcfp_keys_ex[index].htype;
> + struct tcf_pedit_parms *parms;
> + u32 htype = TCA_PEDIT_KEY_EX_HDR_TYPE_NETWORK;
nit: reverse xmas tree for local variables: longest line to shortest
...
> diff --git a/net/sched/act_pedit.c b/net/sched/act_pedit.c
> index a0378e9f0121..1b3499585d7a 100644
> --- a/net/sched/act_pedit.c
> +++ b/net/sched/act_pedit.c
...
> @@ -324,109 +352,107 @@ TC_INDIRECT_SCOPE int tcf_pedit_act(struct sk_buff *skb,
> const struct tc_action *a,
> struct tcf_result *res)
> {
> + enum pedit_header_type htype = TCA_PEDIT_KEY_EX_HDR_TYPE_NETWORK;
> + enum pedit_cmd cmd = TCA_PEDIT_KEY_EX_CMD_SET;
> struct tcf_pedit *p = to_pedit(a);
> + struct tcf_pedit_key_ex *tkey_ex;
> + struct tcf_pedit_parms *parms;
> + struct tc_pedit_key *tkey;
> u32 max_offset;
> int i;
>
> - spin_lock(&p->tcf_lock);
> + parms = rcu_dereference_bh(p->parms);
>
> max_offset = (skb_transport_header_was_set(skb) ?
> skb_transport_offset(skb) :
> skb_network_offset(skb)) +
> - p->tcfp_off_max_hint;
> + parms->tcfp_off_max_hint;
> if (skb_ensure_writable(skb, min(skb->len, max_offset)))
> - goto unlock;
> + goto done;
>
> tcf_lastuse_update(&p->tcf_tm);
> + tcf_action_update_bstats(&p->common, skb);
>
> - if (p->tcfp_nkeys > 0) {
nit: It seems to me that this patch removes the above condition and relies
on: 1) tcfp_nkeys not being negative and 2) the for loop that follows
being a no-op if >tcfp_nkeys is 0.
If so, it is nice as as it reduces indentation and in general
simplifies the code. However, this patch does other things, and making
this indentation change at the same time makes review harder than it
might otherwise be (for me, at least).
Perhaps there is some reason that the changes need to be made at the
same time. But if not, I would have leant towards breaking out the
removal of this condition into a different patch.
> - struct tc_pedit_key *tkey = p->tcfp_keys;
> - struct tcf_pedit_key_ex *tkey_ex = p->tcfp_keys_ex;
> - enum pedit_header_type htype =
> - TCA_PEDIT_KEY_EX_HDR_TYPE_NETWORK;
> - enum pedit_cmd cmd = TCA_PEDIT_KEY_EX_CMD_SET;
> -
> - for (i = p->tcfp_nkeys; i > 0; i--, tkey++) {
> - u32 *ptr, hdata;
> - int offset = tkey->off;
> - int hoffset;
> - u32 val;
> - int rc;
> -
> - if (tkey_ex) {
> - htype = tkey_ex->htype;
> - cmd = tkey_ex->cmd;
> -
> - tkey_ex++;
> - }
> + tkey_ex = parms->tcfp_keys_ex;
> + tkey = parms->tcfp_keys;
>
> - rc = pedit_skb_hdr_offset(skb, htype, &hoffset);
> - if (rc) {
> - pr_info("tc action pedit bad header type specified (0x%x)\n",
> - htype);
> - goto bad;
> - }
> + for (i = parms->tcfp_nkeys; i > 0; i--, tkey++) {
> + u32 *ptr, hdata;
> + int offset = tkey->off;
> + int hoffset;
> + u32 val;
> + int rc;
nit: I appreciate that this is just changing the indentation of existing code.
But it would be nice if it complied to reverse-xmas tree ordering
and consistently either a) had multiple non-assignment declarations on
the same line or b) one declaration per line. e.g.:
int offset = tkey->off;
u32 *ptr, hdata, val;
int hoffset, rc;
Powered by blists - more mailing lists