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

[Pablo Neira Ayuso <pablo@netfilter.org>]

On Fri, Jul 10, 2020 at 05:15:39PM +0200, Petr Machata wrote:
> 
> Pablo Neira Ayuso <pablo@netfilter.org> writes:
> 
> > On Fri, Jul 10, 2020 at 04:56:54PM +0300, Ido Schimmel wrote:
> >> From: Petr Machata <petrm@mellanox.com>
> >>
> >> Previously, shared blocks were only relevant for the pseudo-qdiscs ingress
> >> and clsact. Recently, a qevent facility was introduced, which allows to
> >> bind blocks to well-defined slots of a qdisc instance. RED in particular
> >> got two qevents: early_drop and mark. Drivers that wish to offload these
> >> blocks will be sent the usual notification, and need to know which qdisc it
> >> is related to.
> >>
> >> To that end, extend flow_block_offload with a "sch" pointer, and initialize
> >> as appropriate. This prompts changes in the indirect block facility, which
> >> now tracks the scheduler instead of the netdevice. Update signatures of
> >> several functions similarly. Deduce the device from the scheduler when
> >> necessary.
> >>
> >> Signed-off-by: Petr Machata <petrm@mellanox.com>
> >> Reviewed-by: Jiri Pirko <jiri@mellanox.com>
> >> Signed-off-by: Ido Schimmel <idosch@mellanox.com>
> >> ---
> >>  drivers/net/ethernet/broadcom/bnxt/bnxt_tc.c  | 11 ++++++----
> >>  .../ethernet/mellanox/mlx5/core/en/rep/tc.c   | 11 +++++-----
> >>  .../net/ethernet/netronome/nfp/flower/main.h  |  2 +-
> >>  .../ethernet/netronome/nfp/flower/offload.c   | 11 ++++++----
> >>  include/net/flow_offload.h                    |  9 ++++----
> >>  net/core/flow_offload.c                       | 12 +++++------
> >>  net/netfilter/nf_flow_table_offload.c         | 17 +++++++--------
> >>  net/netfilter/nf_tables_offload.c             | 20 ++++++++++--------
> >>  net/sched/cls_api.c                           | 21 +++++++++++--------
> >>  9 files changed, 63 insertions(+), 51 deletions(-)
> >>
> > [...]
> >> 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?

--- 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.

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);