Re: [PATCH net-next 6/8] net/mlx5e: Introduce tc offload support

Amir Vadai
2016-03-01T17:03:36Z

On Tue, Mar 01, 2016 at 05:59:59PM +0200, Saeed Mahameed wrote:
> On Tue, Mar 1, 2016 at 4:24 PM, Amir Vadai <amir@vadai.me> wrote:
> > +#define FT_CAP(f) MLX5_CAP_FLOWTABLE(mdev, flow_table_properties_nic_receive.f)
> > + if (FT_CAP(flow_modify_en) &&
> > + FT_CAP(modify_root) &&
> > + FT_CAP(identified_miss_table_mode) &&
> > + FT_CAP(flow_table_modify))
> > + priv->netdev->hw_features |= NETIF_F_HW_TC;
> > +
> > netdev->features |= NETIF_F_HIGHDMA;
> >
> > netdev->priv_flags |= IFF_UNICAST_FLT;
> >
> > + mlx5e_tc_init(priv);
>
> This is not the place for this, We usually do internal data structure
> initialization after we create all HW resources in
> mlx5e_create_netdev
> Please see mlx5e_vxlan_init as example, and you already call
> mlx5e_tc_cleanup inside mlx5e_destroy_netdev, please move the
> mlx5e_tc_init
> to mlx5e_create_netdev after HW resources creation,
ack

>
>
> > @@ -2558,6 +2588,7 @@ static void mlx5e_destroy_netdev(struct mlx5_core_dev *mdev, void *vpriv)
> > mlx5_core_dealloc_transport_domain(priv->mdev, priv->tdn);
> > mlx5_core_dealloc_pd(priv->mdev, priv->pdn);
> > mlx5_unmap_free_uar(priv->mdev, &priv->cq_uar);
> > + mlx5e_tc_cleanup(priv);
>
> I would suggest to move mlx5e_tc_init to be right after
> mlx5e_vxlan_init and mlx5e_tc_cleanup before mlx5e_vxlan_cleanup.
ok

>
> > +struct mlx5_flow_rule *mlx5e_tc_add_flow(struct mlx5e_priv *priv,
> > + u32 *match_c, u32 *match_v,
> > + u32 action, u32 flow_tag)
> > +{
> > + struct mlx5_flow_destination dest = {
> > + .type = MLX5_FLOW_DESTINATION_TYPE_FLOW_TABLE,
> > + {.ft = priv->fts.vlan.t},
> > + };
> > + struct mlx5_flow_rule *rule;
> > + bool table_created = false;
> > +
> > + if (IS_ERR_OR_NULL(priv->fts.tc.t)) {
> > + priv->fts.tc.t =
> > + mlx5_create_auto_grouped_flow_table(priv->fts.ns, 0,
> > + MLX5E_TC_FLOW_TABLE_NUM_ENTRIES,
> > + MLX5E_TC_FLOW_TABLE_NUM_GROUPS);
> > + if (IS_ERR(priv->fts.tc.t)) {
> > + netdev_err(priv->netdev,
> > + "Failed to create tc offload table\n");
> > + return ERR_CAST(priv->fts.tc.t);
>
> Here priv->fts.tc.t will be invalid pointer and in your code you treat
> it as NULL in case of failure.
>
> > + }
> > +
> > + table_created = true;
> > + }
> > +
> > + rule = mlx5_add_flow_rule(priv->fts.tc.t, MLX5_MATCH_OUTER_HEADERS,
> > + match_c, match_v,
> > + action, flow_tag,
> > + action & MLX5_FLOW_CONTEXT_ACTION_FWD_DEST ? &dest : NULL);
> > +
> > + if (IS_ERR(rule) && table_created) {
> > + mlx5_destroy_flow_table(priv->fts.tc.t);
> > + priv->fts.tc.t = NULL;
> > + }
> > +
> > + return rule;
> > +}
> > +
>
> > +void mlx5e_tc_cleanup(struct mlx5e_priv *priv)
> > +{
> > + struct mlx5e_tc_flow_table *tc = &priv->fts.tc;
> > +
> > + rhashtable_free_and_destroy(&tc->ht, _mlx5e_tc_del_flow, priv);
> > +
> > + if (priv->fts.tc.t) {
>
> priv->fts.tc.t will be invalid pointer and this test will pass in case
> mlx5_create_auto_grouped_flow_table had failed
Yeh - should have used !IS_ERR_OR_NULL() or set it to NULL on failure
above.

Thanks,
Amir

>
> > + mlx5_destroy_flow_table(priv->fts.tc.t);
> > + priv->fts.tc.t = NULL;
> > + }
> > +}