Re: [PATCH net-next 01/13] net: sched: Pass qdisc reference in struct flow_block_offload

[Petr Machata <petrm@mellanox.com>]

Pablo Neira Ayuso <pablo@netfilter.org> writes:

>> >> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en/rep/tc.c b/drivers/net/ethernet/mellanox/mlx5/core/en/rep/tc.c
>> >> index eefeb1cdc2ee..4fc42c1955ff 100644
>> >> --- a/drivers/net/ethernet/mellanox/mlx5/core/en/rep/tc.c
>> >> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en/rep/tc.c
>> >> @@ -404,7 +404,7 @@ static void mlx5e_rep_indr_block_unbind(void *cb_priv)
>> >>  static LIST_HEAD(mlx5e_block_cb_list);
>> >>
>> >>  static int
>> >> -mlx5e_rep_indr_setup_block(struct net_device *netdev,
>> >> +mlx5e_rep_indr_setup_block(struct Qdisc *sch,
>> >>  			   struct mlx5e_rep_priv *rpriv,
>> >>  			   struct flow_block_offload *f,
>> >>  			   flow_setup_cb_t *setup_cb,
>> >> @@ -412,6 +412,7 @@ mlx5e_rep_indr_setup_block(struct net_device *netdev,
>> >>  			   void (*cleanup)(struct flow_block_cb *block_cb))
>> >>  {
>> >>  	struct mlx5e_priv *priv = netdev_priv(rpriv->netdev);
>> >> +	struct net_device *netdev = sch->dev_queue->dev;
>> >
>> > This break indirect block support for netfilter since the driver
>> > is assuming a Qdisc object.
>>
>> Sorry, I don't follow. You mean mlx5 driver? What does it mean to
>> "assume a qdisc object"?
>>
>> Is it incorrect to rely on the fact that the netdevice can be deduced
>> from a qdisc, or that there is always a qdisc associated with a block
>> binding point?
>
> The drivers assume that the xyz_indr_setup_block() always gets a sch
> object, which is not always true. Are you really sure this will work
> for the TC CT offload?

I tested indirect blocks in general, but not CT offload.

> --- a/net/netfilter/nf_flow_table_offload.c
> +++ b/net/netfilter/nf_flow_table_offload.c
> @@ -928,26 +928,27 @@ static int nf_flow_table_block_setup(struct nf_flowtable *flowtable,
>  }
>
>  static void nf_flow_table_block_offload_init(struct flow_block_offload *bo,
> -                                            struct net *net,
> +                                            struct net_device *dev,
>                                              enum flow_block_command cmd,
>                                              struct nf_flowtable *flowtable,
>                                              struct netlink_ext_ack *extack)
>  {
>         memset(bo, 0, sizeof(*bo));
> -       bo->net         = net;
> +       bo->net         = dev_net(dev);
>         bo->block       = &flowtable->flow_block;
>         bo->command     = cmd;
>         bo->binder_type = FLOW_BLOCK_BINDER_TYPE_CLSACT_INGRESS;
>         bo->extack      = extack;
> +       bo->sch         = dev_ingress_queue(dev)->qdisc_sleeping;
>         INIT_LIST_HEAD(&bo->cb_list);
>  }
>
>  static void nf_flow_table_indr_cleanup(struct flow_block_cb *block_cb)
>  {
>         struct nf_flowtable *flowtable = block_cb->indr.data;
> -       struct net_device *dev = block_cb->indr.dev;
> +       struct Qdisc *sch = block_cb->indr.sch;
>
> -       nf_flow_table_gc_cleanup(flowtable, dev);
> +       nf_flow_table_gc_cleanup(flowtable, sch->dev_queue->dev);
>         down_write(&flowtable->flow_block_lock);
>         list_del(&block_cb->list);
>         list_del(&block_cb->driver_list);
>
> Moreover, the flow_offload infrastructure should also remain
> independent from the front-end, either tc/netfilter/ethtool, this is
> pulling in tc specific stuff into it, eg.

Hmm, OK, so I should not have assumed there is always a qdisc associated
with a blocks.

I'm not sure how strong your objection to pulling in TC is. Would it be
OK, instead of replacing the device with a qdisc in flow_block_indr, to
put in both? The qdisc can be NULL for the "normal" binder types,
because there the block is uniquely identified just by the type. For the
"non-normal" ones it would be obvious how to initialize it.

> diff --git a/include/net/flow_offload.h b/include/net/flow_offload.h
> index de395498440d..fda29140bdc5 100644
> --- a/include/net/flow_offload.h
> +++ b/include/net/flow_offload.h
> @@ -444,6 +444,7 @@ struct flow_block_offload {
>         struct list_head cb_list;
>         struct list_head *driver_block_list;
>         struct netlink_ext_ack *extack;
> +       struct Qdisc *sch;
>  };
>
>  enum tc_setup_type;
> @@ -454,7 +455,7 @@ struct flow_block_cb;
>
>  struct flow_block_indr {
>         struct list_head                list;
> -       struct net_device               *dev;
> +       struct Qdisc                    *sch;
>         enum flow_block_binder_type     binder_type;
>         void                            *data;
>         void                            *cb_priv;
> @@ -479,7 +480,7 @@ struct flow_block_cb *flow_indr_block_cb_alloc(flow_setup_cb_t *cb,
>                                                void *cb_ident, void *cb_priv,
>                                                void (*release)(void *cb_priv),
>                                                struct flow_block_offload *bo,
> -                                              struct net_device *dev, void *data,
> +                                              struct Qdisc *sch, void *data,
>                                                void *indr_cb_priv,
>                                                void (*cleanup)(struct flow_block_cb *block_cb));
>  void flow_block_cb_free(struct flow_block_cb *block_cb);