[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <Y+9maDuUVcqPm5oM@boxer>
Date: Fri, 17 Feb 2023 12:35:04 +0100
From: Maciej Fijalkowski <maciej.fijalkowski@...el.com>
To: Vlad Buslov <vladbu@...dia.com>
CC: Saeed Mahameed <saeed@...nel.org>,
"David S. Miller" <davem@...emloft.net>,
Jakub Kicinski <kuba@...nel.org>,
Paolo Abeni <pabeni@...hat.com>,
Eric Dumazet <edumazet@...gle.com>,
Saeed Mahameed <saeedm@...dia.com>, <netdev@...r.kernel.org>,
Tariq Toukan <tariqt@...dia.com>, Oz Shlomo <ozsh@...dia.com>,
Paul Blakey <paulb@...dia.com>
Subject: Re: [net-next 5/9] net/mlx5e: Implement CT entry update
On Thu, Feb 16, 2023 at 07:15:13PM +0200, Vlad Buslov wrote:
> On Thu 16 Feb 2023 at 16:51, Maciej Fijalkowski <maciej.fijalkowski@...el.com> wrote:
> > On Wed, Feb 15, 2023 at 04:09:14PM -0800, Saeed Mahameed wrote:
> >> From: Vlad Buslov <vladbu@...dia.com>
> >>
> >> With support for UDP NEW offload the flow_table may now send updates for
> >> existing flows. Support properly replacing existing entries by updating
> >> flow restore_cookie and replacing the rule with new one with the same match
> >> but new mod_hdr action that sets updated ctinfo.
> >>
> >> Signed-off-by: Vlad Buslov <vladbu@...dia.com>
> >> Reviewed-by: Oz Shlomo <ozsh@...dia.com>
> >> Reviewed-by: Paul Blakey <paulb@...dia.com>
> >> Signed-off-by: Saeed Mahameed <saeedm@...dia.com>
> >> ---
> >> .../ethernet/mellanox/mlx5/core/en/tc_ct.c | 118 +++++++++++++++++-
> >> 1 file changed, 117 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en/tc_ct.c b/drivers/net/ethernet/mellanox/mlx5/core/en/tc_ct.c
> >> index 193562c14c44..76e86f83b6ac 100644
> >> --- a/drivers/net/ethernet/mellanox/mlx5/core/en/tc_ct.c
> >> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en/tc_ct.c
> >> @@ -871,6 +871,68 @@ mlx5_tc_ct_entry_add_rule(struct mlx5_tc_ct_priv *ct_priv,
> >> return err;
> >> }
> >>
> >> +static int
> >> +mlx5_tc_ct_entry_replace_rule(struct mlx5_tc_ct_priv *ct_priv,
> >> + struct flow_rule *flow_rule,
> >> + struct mlx5_ct_entry *entry,
> >> + bool nat, u8 zone_restore_id)
> >> +{
> >> + struct mlx5_ct_zone_rule *zone_rule = &entry->zone_rules[nat];
> >> + struct mlx5_flow_attr *attr = zone_rule->attr, *old_attr;
> >> + struct mlx5e_mod_hdr_handle *mh;
> >> + struct mlx5_ct_fs_rule *rule;
> >> + struct mlx5_flow_spec *spec;
> >> + int err;
> >> +
> >> + spec = kvzalloc(sizeof(*spec), GFP_KERNEL);
> >> + if (!spec)
> >> + return -ENOMEM;
> >> +
> >> + old_attr = mlx5_alloc_flow_attr(ct_priv->ns_type);
> >> + if (!attr) {
> >
> > when can attr == NULL? maybe check it in the first place before allocing
> > spec above?
>
> Should verify 'old_attr', not 'attr'. Thanks for catching this!
>
> >
> >> + err = -ENOMEM;
> >> + goto err_attr;
> >> + }
> >> + *old_attr = *attr;
> >> +
> >> + err = mlx5_tc_ct_entry_create_mod_hdr(ct_priv, attr, flow_rule, &mh, zone_restore_id,
> >> + nat, mlx5_tc_ct_entry_has_nat(entry));
> >> + if (err) {
> >> + ct_dbg("Failed to create ct entry mod hdr");
> >> + goto err_mod_hdr;
> >> + }
> >> +
> >> + mlx5_tc_ct_set_tuple_match(ct_priv, spec, flow_rule);
> >> + mlx5e_tc_match_to_reg_match(spec, ZONE_TO_REG, entry->tuple.zone, MLX5_CT_ZONE_MASK);
> >> +
> >> + rule = ct_priv->fs_ops->ct_rule_add(ct_priv->fs, spec, attr, flow_rule);
> >> + if (IS_ERR(rule)) {
> >> + err = PTR_ERR(rule);
> >> + ct_dbg("Failed to add replacement ct entry rule, nat: %d", nat);
> >> + goto err_rule;
> >> + }
> >> +
> >> + ct_priv->fs_ops->ct_rule_del(ct_priv->fs, zone_rule->rule);
> >> + zone_rule->rule = rule;
> >> + mlx5_tc_ct_entry_destroy_mod_hdr(ct_priv, old_attr, zone_rule->mh);
> >> + zone_rule->mh = mh;
> >> +
> >> + kfree(old_attr);
> >> + kvfree(spec);
> >
> > not a big deal but you could make a common goto below with a different
> > name
>
> You mean jump from here to the middle of the error handling code below
> in order not to duplicate these two calls to *free()? Honestly, I would
> much rather prefer _not_ to do that since goto is necessary evil for
> error handling in C but I don't believe we should handle any common
> non-error code paths with it and it is not typically done in mlx5.
ok it's fine as-is
>
> >
> >> + ct_dbg("Replaced ct entry rule in zone %d", entry->tuple.zone);
> >> +
> >> + return 0;
> >> +
> >> +err_rule:
> >> + mlx5_tc_ct_entry_destroy_mod_hdr(ct_priv, zone_rule->attr, mh);
> >> + mlx5_put_label_mapping(ct_priv, attr->ct_attr.ct_labels_id);
> >> +err_mod_hdr:
> >> + kfree(old_attr);
> >> +err_attr:
> >> + kvfree(spec);
> >> + return err;
> >> +}
> >> +
> >> static bool
> >> mlx5_tc_ct_entry_valid(struct mlx5_ct_entry *entry)
> >> {
> >> @@ -1065,6 +1127,52 @@ mlx5_tc_ct_entry_add_rules(struct mlx5_tc_ct_priv *ct_priv,
> >> return err;
> >> }
> >>
> >> +static int
> >> +mlx5_tc_ct_entry_replace_rules(struct mlx5_tc_ct_priv *ct_priv,
> >> + struct flow_rule *flow_rule,
> >> + struct mlx5_ct_entry *entry,
> >> + u8 zone_restore_id)
> >> +{
> >> + int err;
> >> +
> >> + err = mlx5_tc_ct_entry_replace_rule(ct_priv, flow_rule, entry, false,
> >
> > would it make sense to replace the bool nat in here with some kind of
> > enum?
>
> It is either nat rule or non-nat rule. Why would we invent an enum here?
> Moreover, boolean 'nat' is already used in multiple places in this file,
> so this patch just re-uses existing convention.
This was just a suggestion to improve readability on callsites and using
bool as an array index was a bit odd for me, but that's related to
personal taste probably.
err = mlx5_tc_ct_entry_replace_rule(ct_priv, flow_rule, entry,
NAT_RULE, zone_restore_id);
if (err)
return err;
err = mlx5_tc_ct_entry_replace_rule(ct_priv, flow_rule, entry,
NON_NAT_RULE, zone_restore_id);
if (err)
return err;
above sheds more light on what is going on instead of opaque true/false.
I didn't realize it's the pattern you're using throughout whole file, so
this change would be weird per-se, maybe consider such refactoring when
you would have some free cycles... or just ignore it:)
> >> + if (err)
> >> + return err;
> >> +
> >> + err = mlx5_tc_ct_entry_replace_rule(ct_priv, flow_rule, entry, true,
> >> + zone_restore_id);
> >> + if (err)
> >> + mlx5_tc_ct_entry_del_rule(ct_priv, entry, false);
> >> + return err;
> >> +}
> >> +
> >> +static int
> >> +mlx5_tc_ct_block_flow_offload_replace(struct mlx5_ct_ft *ft, struct flow_rule *flow_rule,
> >> + struct mlx5_ct_entry *entry, unsigned long cookie)
> >> +{
> >> + struct mlx5_tc_ct_priv *ct_priv = ft->ct_priv;
> >> + int err;
> >> +
> >> + err = mlx5_tc_ct_entry_replace_rules(ct_priv, flow_rule, entry, ft->zone_restore_id);
> >> + if (!err)
> >> + return 0;
> >> +
> >> + /* If failed to update the entry, then look it up again under ht_lock
> >> + * protection and properly delete it.
> >> + */
> >> + spin_lock_bh(&ct_priv->ht_lock);
> >> + entry = rhashtable_lookup_fast(&ft->ct_entries_ht, &cookie, cts_ht_params);
> >> + if (entry) {
> >> + rhashtable_remove_fast(&ft->ct_entries_ht, &entry->node, cts_ht_params);
> >> + spin_unlock_bh(&ct_priv->ht_lock);
> >> + mlx5_tc_ct_entry_put(entry);
> >> + } else {
> >> + spin_unlock_bh(&ct_priv->ht_lock);
> >> + }
> >> + return err;
> >> +}
> >> +
> >> static int
> >> mlx5_tc_ct_block_flow_offload_add(struct mlx5_ct_ft *ft,
> >> struct flow_cls_offload *flow)
> >> @@ -1087,9 +1195,17 @@ mlx5_tc_ct_block_flow_offload_add(struct mlx5_ct_ft *ft,
> >> spin_lock_bh(&ct_priv->ht_lock);
> >> entry = rhashtable_lookup_fast(&ft->ct_entries_ht, &cookie, cts_ht_params);
> >> if (entry && refcount_inc_not_zero(&entry->refcnt)) {
> >> + if (entry->restore_cookie == meta_action->ct_metadata.cookie) {
> >> + spin_unlock_bh(&ct_priv->ht_lock);
> >> + mlx5_tc_ct_entry_put(entry);
> >> + return -EEXIST;
> >> + }
> >> + entry->restore_cookie = meta_action->ct_metadata.cookie;
> >> spin_unlock_bh(&ct_priv->ht_lock);
> >> +
> >> + err = mlx5_tc_ct_block_flow_offload_replace(ft, flow_rule, entry, cookie);
> >> mlx5_tc_ct_entry_put(entry);
> >
> > in case of err != 0, haven't you already put the entry inside
> > mlx5_tc_ct_block_flow_offload_replace() ?
>
> No. Here we release the reference that was obtained 10 lines up and
> mlx5_tc_ct_block_flow_offload_replace() releases the reference held by
> ct_entries_ht table.
thanks for explanation
>
> >
> >> - return -EEXIST;
> >> + return err;
> >> }
> >> spin_unlock_bh(&ct_priv->ht_lock);
> >>
> >> --
> >> 2.39.1
> >>
>
Powered by blists - more mailing lists