diff mbox series

[net-next,v8,1/5] net/sched: Introduce tc block netdev tracking infra

Message ID 20231219181623.3845083-2-victor@mojatatu.com (mailing list archive)
State Accepted
Commit 913b47d3424e7d99eaf34b798c47dfa840c64a08
Delegated to: Netdev Maintainers
Headers show
Series net/sched: Introduce tc block ports tracking and use | expand

Checks

Context Check Description
netdev/series_format success Posting correctly formatted
netdev/tree_selection success Clearly marked for net-next
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 2587 this patch: 2587
netdev/cc_maintainers success CCed 7 of 7 maintainers
netdev/build_clang fail Errors and warnings before: 134 this patch: 134
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 2681 this patch: 2681
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 115 lines checked
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Victor Nogueira Dec. 19, 2023, 6:16 p.m. UTC 
This commit makes tc blocks track which ports have been added to them.
And, with that, we'll be able to use this new information to send
packets to the block's ports. Which will be done in the patch #3 of this
series.

Suggested-by: Jiri Pirko <jiri@nvidia.com>
Co-developed-by: Jamal Hadi Salim <jhs@mojatatu.com>
Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>
Co-developed-by: Pedro Tammela <pctammela@mojatatu.com>
Signed-off-by: Pedro Tammela <pctammela@mojatatu.com>
Signed-off-by: Victor Nogueira <victor@mojatatu.com>
---
 include/net/sch_generic.h |  2 ++
 net/sched/cls_api.c       |  2 ++
 net/sched/sch_api.c       | 41 +++++++++++++++++++++++++++++++++++++++
 net/sched/sch_generic.c   | 18 ++++++++++++++++-
 4 files changed, 62 insertions(+), 1 deletion(-)

Comments

Ido Schimmel Dec. 28, 2023, 11:40 a.m. UTC | #1 
On Tue, Dec 19, 2023 at 03:16:19PM -0300, Victor Nogueira wrote:
> +static int qdisc_block_add_dev(struct Qdisc *sch, struct net_device *dev,
> +			       struct netlink_ext_ack *extack)
> +{
> +	const struct Qdisc_class_ops *cl_ops = sch->ops->cl_ops;
> +	struct tcf_block *block;
> +	int err;
> +
> +	block = cl_ops->tcf_block(sch, TC_H_MIN_INGRESS, NULL);
> +	if (block) {
> +		err = xa_insert(&block->ports, dev->ifindex, dev, GFP_KERNEL);
> +		if (err) {
> +			NL_SET_ERR_MSG(extack,
> +				       "ingress block dev insert failed");
> +			return err;
> +		}
> +	}
> +
> +	block = cl_ops->tcf_block(sch, TC_H_MIN_EGRESS, NULL);
> +	if (block) {
> +		err = xa_insert(&block->ports, dev->ifindex, dev, GFP_KERNEL);
> +		if (err) {
> +			NL_SET_ERR_MSG(extack,
> +				       "Egress block dev insert failed");
> +			goto err_out;
> +		}
> +	}

The following fails after this patch:

# tc qdisc add dev swp1 ingress
Error: Egress block dev insert failed.

Probably because ingress_tcf_block() ignores the 'cl' argument.

> +
> +	return 0;
> +
> +err_out:
> +	block = cl_ops->tcf_block(sch, TC_H_MIN_INGRESS, NULL);
> +	if (block)
> +		xa_erase(&block->ports, dev->ifindex);
> +
> +	return err;
> +}
Ido Schimmel Dec. 28, 2023, 11:50 a.m. UTC | #2 
On Tue, Dec 19, 2023 at 03:16:19PM -0300, Victor Nogueira wrote:
> +static int qdisc_block_add_dev(struct Qdisc *sch, struct net_device *dev,
> +			       struct netlink_ext_ack *extack)
> +{
> +	const struct Qdisc_class_ops *cl_ops = sch->ops->cl_ops;
> +	struct tcf_block *block;
> +	int err;
> +
> +	block = cl_ops->tcf_block(sch, TC_H_MIN_INGRESS, NULL);

Another problem, shouldn't there be a check that these operations are
actually implemented? The following now crashes with a NULL pointer
dereference:

# tc qdisc replace dev swp1 root handle 1: tbf rate 1Mbit burst 256k limit 1M

> +	if (block) {
> +		err = xa_insert(&block->ports, dev->ifindex, dev, GFP_KERNEL);
> +		if (err) {
> +			NL_SET_ERR_MSG(extack,
> +				       "ingress block dev insert failed");
> +			return err;
> +		}
> +	}
> +
> +	block = cl_ops->tcf_block(sch, TC_H_MIN_EGRESS, NULL);
> +	if (block) {
> +		err = xa_insert(&block->ports, dev->ifindex, dev, GFP_KERNEL);
> +		if (err) {
> +			NL_SET_ERR_MSG(extack,
> +				       "Egress block dev insert failed");
> +			goto err_out;
> +		}
> +	}
> +
> +	return 0;
> +
> +err_out:
> +	block = cl_ops->tcf_block(sch, TC_H_MIN_INGRESS, NULL);
> +	if (block)
> +		xa_erase(&block->ports, dev->ifindex);
> +
> +	return err;
> +}
> +
>  static int qdisc_block_indexes_set(struct Qdisc *sch, struct nlattr **tca,
>  				   struct netlink_ext_ack *extack)
>  {
> @@ -1350,6 +1387,10 @@ static struct Qdisc *qdisc_create(struct net_device *dev,
>  	qdisc_hash_add(sch, false);
>  	trace_qdisc_create(ops, dev, parent);
>  
> +	err = qdisc_block_add_dev(sch, dev, extack);
> +	if (err)
> +		goto err_out4;
> +
>  	return sch;
>  
>  err_out4:
Jamal Hadi Salim Dec. 28, 2023, 12:35 p.m. UTC | #3 
On Thu, Dec 28, 2023 at 6:50 AM Ido Schimmel <idosch@idosch.org> wrote:
>
> On Tue, Dec 19, 2023 at 03:16:19PM -0300, Victor Nogueira wrote:
> > +static int qdisc_block_add_dev(struct Qdisc *sch, struct net_device *dev,
> > +                            struct netlink_ext_ack *extack)
> > +{
> > +     const struct Qdisc_class_ops *cl_ops = sch->ops->cl_ops;
> > +     struct tcf_block *block;
> > +     int err;
> > +
> > +     block = cl_ops->tcf_block(sch, TC_H_MIN_INGRESS, NULL);
>
> Another problem, shouldn't there be a check that these operations are
> actually implemented? The following now crashes with a NULL pointer
> dereference:
>
> # tc qdisc replace dev swp1 root handle 1: tbf rate 1Mbit burst 256k limit 1M


I think this broke from v7->v8. Thanks for catching this. We'll send a
fix shortly.

cheers,
jamal

> > +     if (block) {
> > +             err = xa_insert(&block->ports, dev->ifindex, dev, GFP_KERNEL);
> > +             if (err) {
> > +                     NL_SET_ERR_MSG(extack,
> > +                                    "ingress block dev insert failed");
> > +                     return err;
> > +             }
> > +     }
> > +
> > +     block = cl_ops->tcf_block(sch, TC_H_MIN_EGRESS, NULL);
> > +     if (block) {
> > +             err = xa_insert(&block->ports, dev->ifindex, dev, GFP_KERNEL);
> > +             if (err) {
> > +                     NL_SET_ERR_MSG(extack,
> > +                                    "Egress block dev insert failed");
> > +                     goto err_out;
> > +             }
> > +     }
> > +
> > +     return 0;
> > +
> > +err_out:
> > +     block = cl_ops->tcf_block(sch, TC_H_MIN_INGRESS, NULL);
> > +     if (block)
> > +             xa_erase(&block->ports, dev->ifindex);
> > +
> > +     return err;
> > +}
> > +
> >  static int qdisc_block_indexes_set(struct Qdisc *sch, struct nlattr **tca,
> >                                  struct netlink_ext_ack *extack)
> >  {
> > @@ -1350,6 +1387,10 @@ static struct Qdisc *qdisc_create(struct net_device *dev,
> >       qdisc_hash_add(sch, false);
> >       trace_qdisc_create(ops, dev, parent);
> >
> > +     err = qdisc_block_add_dev(sch, dev, extack);
> > +     if (err)
> > +             goto err_out4;
> > +
> >       return sch;
> >
> >  err_out4:
Victor Nogueira Dec. 28, 2023, 2:13 p.m. UTC | #4 
On 28/12/2023 09:35, Jamal Hadi Salim wrote:
> On Thu, Dec 28, 2023 at 6:50 AM Ido Schimmel <idosch@idosch.org> wrote:
>>
>> On Tue, Dec 19, 2023 at 03:16:19PM -0300, Victor Nogueira wrote:
>>> +static int qdisc_block_add_dev(struct Qdisc *sch, struct net_device *dev,
>>> +                            struct netlink_ext_ack *extack)
>>> +{
>>> +     const struct Qdisc_class_ops *cl_ops = sch->ops->cl_ops;
>>> +     struct tcf_block *block;
>>> +     int err;
>>> +
>>> +     block = cl_ops->tcf_block(sch, TC_H_MIN_INGRESS, NULL);
>>
>> Another problem, shouldn't there be a check that these operations are
>> actually implemented? The following now crashes with a NULL pointer
>> dereference:
>>
>> # tc qdisc replace dev swp1 root handle 1: tbf rate 1Mbit burst 256k limit 1M
> 
> 
> I think this broke from v7->v8. Thanks for catching this. We'll send a
> fix shortly.

Just sent a fix to net-next because the original patch hasn't been
propagated to net yet.

cheers,
Victor
diff mbox series

Patch

diff --git a/include/net/sch_generic.h b/include/net/sch_generic.h
index dcb9160e6467..248692ec3697 100644
--- a/include/net/sch_generic.h
+++ b/include/net/sch_generic.h
@@ -19,6 +19,7 @@ 
 #include <net/gen_stats.h>
 #include <net/rtnetlink.h>
 #include <net/flow_offload.h>
+#include <linux/xarray.h>
 
 struct Qdisc_ops;
 struct qdisc_walker;
@@ -457,6 +458,7 @@  struct tcf_chain {
 };
 
 struct tcf_block {
+	struct xarray ports; /* datapath accessible */
 	/* Lock protects tcf_block and lifetime-management data of chains
 	 * attached to the block (refcnt, action_refcnt, explicitly_created).
 	 */
diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
index dc1c19a25882..6020a32ecff2 100644
--- a/net/sched/cls_api.c
+++ b/net/sched/cls_api.c
@@ -531,6 +531,7 @@  static void tcf_block_destroy(struct tcf_block *block)
 {
 	mutex_destroy(&block->lock);
 	mutex_destroy(&block->proto_destroy_lock);
+	xa_destroy(&block->ports);
 	kfree_rcu(block, rcu);
 }
 
@@ -1002,6 +1003,7 @@  static struct tcf_block *tcf_block_create(struct net *net, struct Qdisc *q,
 	refcount_set(&block->refcnt, 1);
 	block->net = net;
 	block->index = block_index;
+	xa_init(&block->ports);
 
 	/* Don't store q pointer for blocks which are shared */
 	if (!tcf_block_shared(block))
diff --git a/net/sched/sch_api.c b/net/sched/sch_api.c
index e9eaf637220e..299086bb6205 100644
--- a/net/sched/sch_api.c
+++ b/net/sched/sch_api.c
@@ -1180,6 +1180,43 @@  static int qdisc_graft(struct net_device *dev, struct Qdisc *parent,
 	return 0;
 }
 
+static int qdisc_block_add_dev(struct Qdisc *sch, struct net_device *dev,
+			       struct netlink_ext_ack *extack)
+{
+	const struct Qdisc_class_ops *cl_ops = sch->ops->cl_ops;
+	struct tcf_block *block;
+	int err;
+
+	block = cl_ops->tcf_block(sch, TC_H_MIN_INGRESS, NULL);
+	if (block) {
+		err = xa_insert(&block->ports, dev->ifindex, dev, GFP_KERNEL);
+		if (err) {
+			NL_SET_ERR_MSG(extack,
+				       "ingress block dev insert failed");
+			return err;
+		}
+	}
+
+	block = cl_ops->tcf_block(sch, TC_H_MIN_EGRESS, NULL);
+	if (block) {
+		err = xa_insert(&block->ports, dev->ifindex, dev, GFP_KERNEL);
+		if (err) {
+			NL_SET_ERR_MSG(extack,
+				       "Egress block dev insert failed");
+			goto err_out;
+		}
+	}
+
+	return 0;
+
+err_out:
+	block = cl_ops->tcf_block(sch, TC_H_MIN_INGRESS, NULL);
+	if (block)
+		xa_erase(&block->ports, dev->ifindex);
+
+	return err;
+}
+
 static int qdisc_block_indexes_set(struct Qdisc *sch, struct nlattr **tca,
 				   struct netlink_ext_ack *extack)
 {
@@ -1350,6 +1387,10 @@  static struct Qdisc *qdisc_create(struct net_device *dev,
 	qdisc_hash_add(sch, false);
 	trace_qdisc_create(ops, dev, parent);
 
+	err = qdisc_block_add_dev(sch, dev, extack);
+	if (err)
+		goto err_out4;
+
 	return sch;
 
 err_out4:
diff --git a/net/sched/sch_generic.c b/net/sched/sch_generic.c
index 8dd0e5925342..e33568df97a5 100644
--- a/net/sched/sch_generic.c
+++ b/net/sched/sch_generic.c
@@ -1051,6 +1051,9 @@  static void qdisc_free_cb(struct rcu_head *head)
 static void __qdisc_destroy(struct Qdisc *qdisc)
 {
 	const struct Qdisc_ops  *ops = qdisc->ops;
+	struct net_device *dev = qdisc_dev(qdisc);
+	const struct Qdisc_class_ops *cops;
+	struct tcf_block *block;
 
 #ifdef CONFIG_NET_SCHED
 	qdisc_hash_del(qdisc);
@@ -1061,11 +1064,24 @@  static void __qdisc_destroy(struct Qdisc *qdisc)
 
 	qdisc_reset(qdisc);
 
+	cops = ops->cl_ops;
+	if (ops->ingress_block_get) {
+		block = cops->tcf_block(qdisc, TC_H_MIN_INGRESS, NULL);
+		if (block)
+			xa_erase(&block->ports, dev->ifindex);
+	}
+
+	if (ops->egress_block_get) {
+		block = cops->tcf_block(qdisc, TC_H_MIN_EGRESS, NULL);
+		if (block)
+			xa_erase(&block->ports, dev->ifindex);
+	}
+
 	if (ops->destroy)
 		ops->destroy(qdisc);
 
 	module_put(ops->owner);
-	netdev_put(qdisc_dev(qdisc), &qdisc->dev_tracker);
+	netdev_put(dev, &qdisc->dev_tracker);
 
 	trace_qdisc_destroy(qdisc);