* Re: [PATCH net] sch_htb: Fix inconsistency when leaf qdisc creation fails
@ 2021-08-31 13:52 kernel test robot
0 siblings, 0 replies; 5+ messages in thread
From: kernel test robot @ 2021-08-31 13:52 UTC (permalink / raw)
To: kbuild
[-- Attachment #1: Type: text/plain, Size: 14421 bytes --]
CC: llvm(a)lists.linux.dev
CC: kbuild-all(a)lists.01.org
In-Reply-To: <20210826115425.1744053-1-maximmi@nvidia.com>
References: <20210826115425.1744053-1-maximmi@nvidia.com>
TO: Maxim Mikityanskiy <maximmi@nvidia.com>
Hi Maxim,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on net/master]
url: https://github.com/0day-ci/linux/commits/Maxim-Mikityanskiy/sch_htb-Fix-inconsistency-when-leaf-qdisc-creation-fails/20210826-195513
base: https://git.kernel.org/pub/scm/linux/kernel/git/davem/net.git a423cbe0f21353ac1e63aad037fd5ccf446440bc
:::::: branch date: 5 days ago
:::::: commit date: 5 days ago
config: powerpc-randconfig-c003-20210830 (attached as .config)
compiler: clang version 14.0.0 (https://github.com/llvm/llvm-project 4b1fde8a2b681dad2ce0c082a5d6422caa06b0bc)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# install powerpc cross compiling tool for clang build
# apt-get install binutils-powerpc-linux-gnu
# https://github.com/0day-ci/linux/commit/b518aca0667deecd4ac3466320676b26c9586068
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Maxim-Mikityanskiy/sch_htb-Fix-inconsistency-when-leaf-qdisc-creation-fails/20210826-195513
git checkout b518aca0667deecd4ac3466320676b26c9586068
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=powerpc clang-analyzer
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
clang-analyzer warnings: (new ones prefixed by >>)
^
net/nfc/nci/data.c:117:4: note: Assuming '__UNIQUE_ID___x489' is >= '__UNIQUE_ID___y490'
min_t(int, total_len, conn_info->max_pkt_payload_len);
^
include/linux/minmax.h:104:27: note: expanded from macro 'min_t'
#define min_t(type, x, y) __careful_cmp((type)(x), (type)(y), <)
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
include/linux/minmax.h:38:3: note: expanded from macro '__careful_cmp'
__cmp_once(x, y, __UNIQUE_ID(__x), __UNIQUE_ID(__y), op))
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
include/linux/minmax.h:33:3: note: expanded from macro '__cmp_once'
__cmp(unique_x, unique_y, op); })
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
include/linux/minmax.h:28:26: note: expanded from macro '__cmp'
#define __cmp(x, y, op) ((x) op (y) ? (x) : (y))
^~~~~~~~~~
net/nfc/nci/data.c:117:4: note: '?' condition is false
min_t(int, total_len, conn_info->max_pkt_payload_len);
^
include/linux/minmax.h:104:27: note: expanded from macro 'min_t'
#define min_t(type, x, y) __careful_cmp((type)(x), (type)(y), <)
^
include/linux/minmax.h:38:3: note: expanded from macro '__careful_cmp'
__cmp_once(x, y, __UNIQUE_ID(__x), __UNIQUE_ID(__y), op))
^
include/linux/minmax.h:33:3: note: expanded from macro '__cmp_once'
__cmp(unique_x, unique_y, op); })
^
include/linux/minmax.h:28:26: note: expanded from macro '__cmp'
#define __cmp(x, y, op) ((x) op (y) ? (x) : (y))
^
net/nfc/nci/data.c:122:7: note: 'skb_frag' is not equal to NULL
if (skb_frag == NULL) {
^~~~~~~~
net/nfc/nci/data.c:122:3: note: Taking false branch
if (skb_frag == NULL) {
^
net/nfc/nci/data.c:133:9: note: Assuming 'total_len' is not equal to 'frag_len'
((total_len == frag_len) ?
^~~~~~~~~~~~~~~~~~~~~
net/nfc/nci/data.c:133:8: note: '?' condition is false
((total_len == frag_len) ?
^
net/nfc/nci/data.c:136:3: note: Calling '__skb_queue_tail'
__skb_queue_tail(&frags_q, skb_frag);
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
include/linux/skbuff.h:2076:2: note: Calling '__skb_queue_before'
__skb_queue_before(list, (struct sk_buff *)list, newsk);
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
include/linux/skbuff.h:2043:2: note: 2nd function call argument is an uninitialized value
__skb_insert(newsk, next->prev, next, list);
^ ~~~~~~~~~~
Suppressed 8 warnings (2 in non-user code, 6 with check filters).
Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.
9 warnings generated.
drivers/usb/core/message.c:1709:2: warning: Value stored to 'retval' is never read [clang-analyzer-deadcode.DeadStores]
retval = 0;
^ ~
drivers/usb/core/message.c:1709:2: note: Value stored to 'retval' is never read
retval = 0;
^ ~
Suppressed 8 warnings (2 in non-user code, 6 with check filters).
Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.
8 warnings generated.
Suppressed 8 warnings (2 in non-user code, 6 with check filters).
Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.
8 warnings generated.
Suppressed 8 warnings (2 in non-user code, 6 with check filters).
Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.
8 warnings generated.
Suppressed 8 warnings (2 in non-user code, 6 with check filters).
Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.
8 warnings generated.
Suppressed 8 warnings (2 in non-user code, 6 with check filters).
Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.
8 warnings generated.
Suppressed 8 warnings (2 in non-user code, 6 with check filters).
Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.
8 warnings generated.
Suppressed 8 warnings (2 in non-user code, 6 with check filters).
Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.
10 warnings generated.
Suppressed 10 warnings (2 in non-user code, 8 with check filters).
Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.
8 warnings generated.
Suppressed 8 warnings (2 in non-user code, 6 with check filters).
Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.
8 warnings generated.
Suppressed 8 warnings (2 in non-user code, 6 with check filters).
Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.
8 warnings generated.
Suppressed 8 warnings (2 in non-user code, 6 with check filters).
Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.
8 warnings generated.
Suppressed 8 warnings (2 in non-user code, 6 with check filters).
Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.
8 warnings generated.
Suppressed 8 warnings (2 in non-user code, 6 with check filters).
Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.
11 warnings generated.
>> net/sched/sch_htb.c:1420:8: warning: Access to field 'flags' results in a dereference of a null pointer (loaded from field 'q') [clang-analyzer-core.NullDereference]
if (!(cl->leaf.q->flags & TCQ_F_BUILTIN))
^
net/sched/sch_htb.c:1716:6: note: Assuming field 'children' is 0
if (cl->children || cl->filter_cnt)
^~~~~~~~~~~~
net/sched/sch_htb.c:1716:6: note: Left side of '||' is false
net/sched/sch_htb.c:1716:22: note: Assuming field 'filter_cnt' is 0
if (cl->children || cl->filter_cnt)
^~~~~~~~~~~~~~
net/sched/sch_htb.c:1716:2: note: Taking false branch
if (cl->children || cl->filter_cnt)
^
net/sched/sch_htb.c:1719:6: note: Assuming field 'level' is 0
if (!cl->level && htb_parent_last_child(cl))
^~~~~~~~~~
net/sched/sch_htb.c:1719:6: note: Left side of '&&' is true
net/sched/sch_htb.c:1719:20: note: Calling 'htb_parent_last_child'
if (!cl->level && htb_parent_last_child(cl))
^~~~~~~~~~~~~~~~~~~~~~~~~
net/sched/sch_htb.c:1511:6: note: Assuming field 'parent' is null
if (!cl->parent)
^~~~~~~~~~~
net/sched/sch_htb.c:1511:2: note: Taking true branch
if (!cl->parent)
^
net/sched/sch_htb.c:1513:3: note: Returning without writing to 'cl->.leaf.q'
return 0;
^
net/sched/sch_htb.c:1719:20: note: Returning from 'htb_parent_last_child'
if (!cl->level && htb_parent_last_child(cl))
^~~~~~~~~~~~~~~~~~~~~~~~~
net/sched/sch_htb.c:1719:2: note: Taking false branch
if (!cl->level && htb_parent_last_child(cl))
^
net/sched/sch_htb.c:1722:6: note: Assuming field 'offload' is true
if (q->offload) {
^~~~~~~~~~
net/sched/sch_htb.c:1722:2: note: Taking true branch
if (q->offload) {
^
net/sched/sch_htb.c:1723:9: note: Calling 'htb_destroy_class_offload'
err = htb_destroy_class_offload(sch, cl, last_child, false,
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
net/sched/sch_htb.c:1566:10: note: Field 'level' is 0
if (cl->level)
^
net/sched/sch_htb.c:1566:2: note: Taking false branch
if (cl->level)
^
net/sched/sch_htb.c:1569:10: note: Assuming 'q' is null
WARN_ON(!q);
^
include/asm-generic/bug.h:166:25: note: expanded from macro 'WARN_ON'
int __ret_warn_on = !!(condition); \
^~~~~~~~~
net/sched/sch_htb.c:1570:14: note: Calling 'htb_offload_get_queue'
dev_queue = htb_offload_get_queue(cl);
^~~~~~~~~~~~~~~~~~~~~~~~~
net/sched/sch_htb.c:1420:8: note: Access to field 'flags' results in a dereference of a null pointer (loaded from field 'q')
if (!(cl->leaf.q->flags & TCQ_F_BUILTIN))
^ ~
net/sched/sch_htb.c:1489:17: warning: The left operand of '!=' is a garbage value [clang-analyzer-core.UndefinedBinaryOperatorResult]
WARN_ON(old_q != *old);
^
include/asm-generic/bug.h:166:25: note: expanded from macro 'WARN_ON'
int __ret_warn_on = !!(condition); \
^~~~~~~~~
net/sched/sch_htb.c:1464:2: note: 'old_q' declared without an initial value
struct Qdisc *old_q;
^~~~~~~~~~~~~~~~~~~
net/sched/sch_htb.c:1466:6: note: Assuming field 'level' is 0
if (cl->level)
^~~~~~~~~
net/sched/sch_htb.c:1466:2: note: Taking false branch
if (cl->level)
^
net/sched/sch_htb.c:1469:6: note: Assuming field 'offload' is false
if (q->offload)
^~~~~~~~~~
net/sched/sch_htb.c:1469:2: note: Taking false branch
if (q->offload)
^
net/sched/sch_htb.c:1472:6: note: Assuming 'new' is non-null
if (!new) {
^~~~
net/sched/sch_htb.c:1472:2: note: Taking false branch
if (!new) {
^
net/sched/sch_htb.c:1479:9: note: Field 'offload' is false
if (q->offload) {
^
net/sched/sch_htb.c:1479:2: note: Taking false branch
if (q->offload) {
^
net/sched/sch_htb.c:1486:9: note: Calling 'qdisc_replace'
*old = qdisc_replace(sch, new, &cl->leaf.q);
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
include/net/sch_generic.h:1203:6: note: Assuming 'old' is equal to NULL
if (old != NULL)
^~~~~~~~~~~
vim +1420 net/sched/sch_htb.c
d03b195b5aa015 Maxim Mikityanskiy 2021-01-19 1414
b518aca0667dee Maxim Mikityanskiy 2021-08-26 1415 static struct netdev_queue *htb_offload_get_queue(struct htb_class *cl)
b518aca0667dee Maxim Mikityanskiy 2021-08-26 1416 {
b518aca0667dee Maxim Mikityanskiy 2021-08-26 1417 struct netdev_queue *queue;
b518aca0667dee Maxim Mikityanskiy 2021-08-26 1418
b518aca0667dee Maxim Mikityanskiy 2021-08-26 1419 queue = cl->leaf.offload_queue;
b518aca0667dee Maxim Mikityanskiy 2021-08-26 @1420 if (!(cl->leaf.q->flags & TCQ_F_BUILTIN))
b518aca0667dee Maxim Mikityanskiy 2021-08-26 1421 WARN_ON(cl->leaf.q->dev_queue != queue);
b518aca0667dee Maxim Mikityanskiy 2021-08-26 1422
b518aca0667dee Maxim Mikityanskiy 2021-08-26 1423 return queue;
b518aca0667dee Maxim Mikityanskiy 2021-08-26 1424 }
b518aca0667dee Maxim Mikityanskiy 2021-08-26 1425
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org
[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 30885 bytes --]
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH net] sch_htb: Fix inconsistency when leaf qdisc creation fails
2021-08-26 11:54 Maxim Mikityanskiy
2021-08-26 16:10 ` Eric Dumazet
@ 2021-08-30 23:50 ` patchwork-bot+netdevbpf
1 sibling, 0 replies; 5+ messages in thread
From: patchwork-bot+netdevbpf @ 2021-08-30 23:50 UTC (permalink / raw)
To: Maxim Mikityanskiy
Cc: kuba, davem, jhs, xiyou.wangcong, jiri, saeedm, tariqt, netdev
Hello:
This patch was applied to netdev/net-next.git (refs/heads/master):
On Thu, 26 Aug 2021 14:54:25 +0300 you wrote:
> In HTB offload mode, qdiscs of leaf classes are grafted to netdev
> queues. sch_htb expects the dev_queue field of these qdiscs to point to
> the corresponding queues. However, qdisc creation may fail, and in that
> case noop_qdisc is used instead. Its dev_queue doesn't point to the
> right queue, so sch_htb can lose track of used netdev queues, which will
> cause internal inconsistencies.
>
> [...]
Here is the summary with links:
- [net] sch_htb: Fix inconsistency when leaf qdisc creation fails
https://git.kernel.org/netdev/net-next/c/ca49bfd90a9d
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH net] sch_htb: Fix inconsistency when leaf qdisc creation fails
2021-08-26 16:10 ` Eric Dumazet
@ 2021-08-26 17:42 ` Saeed Mahameed
0 siblings, 0 replies; 5+ messages in thread
From: Saeed Mahameed @ 2021-08-26 17:42 UTC (permalink / raw)
To: Maxim Mikityanskiy, jhs, jiri, eric.dumazet, kuba, xiyou.wangcong, davem
Cc: Tariq Toukan, netdev
On Thu, 2021-08-26 at 09:10 -0700, Eric Dumazet wrote:
>
>
> On 8/26/21 4:54 AM, Maxim Mikityanskiy wrote:
> > In HTB offload mode, qdiscs of leaf classes are grafted to netdev
> > queues. sch_htb expects the dev_queue field of these qdiscs to
> > point to
> > the corresponding queues. However, qdisc creation may fail, and in
> > that
> > case noop_qdisc is used instead. Its dev_queue doesn't point to the
> > right queue, so sch_htb can lose track of used netdev queues, which
> > will
> > cause internal inconsistencies.
> >
> > This commit fixes this bug by keeping track of the netdev queue
> > inside
> > struct htb_class. All reads of cl->leaf.q->dev_queue are replaced
> > by the
> > new field, the two values are synced on writes, and WARNs are added
> > to
> > assert equality of the two values.
> >
> > The driver API has changed: when TC_HTB_LEAF_DEL needs to move a
> > queue,
> > the driver used to pass the old and new queue IDs to sch_htb. Now
> > that
> > there is a new field (offload_queue) in struct htb_class that needs
> > to
> > be updated on this operation, the driver will pass the old class ID
> > to
> > sch_htb instead (it already knows the new class ID).
> >
> > Fixes: d03b195b5aa0 ("sch_htb: Hierarchical QoS hardware offload")
> > Signed-off-by: Maxim Mikityanskiy <maximmi@nvidia.com>
> > Reviewed-by: Tariq Toukan <tariqt@nvidia.com>
> > ---
> > .../net/ethernet/mellanox/mlx5/core/en/qos.c | 15 ++-
> > .../net/ethernet/mellanox/mlx5/core/en/qos.h | 4 +-
> > .../net/ethernet/mellanox/mlx5/core/en_main.c | 3 +-
> > include/net/pkt_cls.h | 3 +-
> > net/sched/sch_htb.c | 97 ++++++++++++---
> > ----
> > 5 files changed, 72 insertions(+), 50 deletions(-)
>
> Having one patch touching net/sched and one driver looks odd.
>
> I guess it is not possible to split it ?
>
not really possible since API for the driver got changed
struct tc_htb_qopt_offload: removed moved_qid field.
we can minimize the driver code just to make it build, then implement
the proper support in a later patch, but what's the point, the driver
changes are pretty minimal.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH net] sch_htb: Fix inconsistency when leaf qdisc creation fails
2021-08-26 11:54 Maxim Mikityanskiy
@ 2021-08-26 16:10 ` Eric Dumazet
2021-08-26 17:42 ` Saeed Mahameed
2021-08-30 23:50 ` patchwork-bot+netdevbpf
1 sibling, 1 reply; 5+ messages in thread
From: Eric Dumazet @ 2021-08-26 16:10 UTC (permalink / raw)
To: Maxim Mikityanskiy, Jakub Kicinski, David S. Miller,
Jamal Hadi Salim, Cong Wang, Jiri Pirko
Cc: Saeed Mahameed, Tariq Toukan, netdev
On 8/26/21 4:54 AM, Maxim Mikityanskiy wrote:
> In HTB offload mode, qdiscs of leaf classes are grafted to netdev
> queues. sch_htb expects the dev_queue field of these qdiscs to point to
> the corresponding queues. However, qdisc creation may fail, and in that
> case noop_qdisc is used instead. Its dev_queue doesn't point to the
> right queue, so sch_htb can lose track of used netdev queues, which will
> cause internal inconsistencies.
>
> This commit fixes this bug by keeping track of the netdev queue inside
> struct htb_class. All reads of cl->leaf.q->dev_queue are replaced by the
> new field, the two values are synced on writes, and WARNs are added to
> assert equality of the two values.
>
> The driver API has changed: when TC_HTB_LEAF_DEL needs to move a queue,
> the driver used to pass the old and new queue IDs to sch_htb. Now that
> there is a new field (offload_queue) in struct htb_class that needs to
> be updated on this operation, the driver will pass the old class ID to
> sch_htb instead (it already knows the new class ID).
>
> Fixes: d03b195b5aa0 ("sch_htb: Hierarchical QoS hardware offload")
> Signed-off-by: Maxim Mikityanskiy <maximmi@nvidia.com>
> Reviewed-by: Tariq Toukan <tariqt@nvidia.com>
> ---
> .../net/ethernet/mellanox/mlx5/core/en/qos.c | 15 ++-
> .../net/ethernet/mellanox/mlx5/core/en/qos.h | 4 +-
> .../net/ethernet/mellanox/mlx5/core/en_main.c | 3 +-
> include/net/pkt_cls.h | 3 +-
> net/sched/sch_htb.c | 97 ++++++++++++-------
> 5 files changed, 72 insertions(+), 50 deletions(-)
Having one patch touching net/sched and one driver looks odd.
I guess it is not possible to split it ?
^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH net] sch_htb: Fix inconsistency when leaf qdisc creation fails
@ 2021-08-26 11:54 Maxim Mikityanskiy
2021-08-26 16:10 ` Eric Dumazet
2021-08-30 23:50 ` patchwork-bot+netdevbpf
0 siblings, 2 replies; 5+ messages in thread
From: Maxim Mikityanskiy @ 2021-08-26 11:54 UTC (permalink / raw)
To: Jakub Kicinski, David S. Miller, Jamal Hadi Salim, Cong Wang, Jiri Pirko
Cc: Saeed Mahameed, Tariq Toukan, netdev, Maxim Mikityanskiy
In HTB offload mode, qdiscs of leaf classes are grafted to netdev
queues. sch_htb expects the dev_queue field of these qdiscs to point to
the corresponding queues. However, qdisc creation may fail, and in that
case noop_qdisc is used instead. Its dev_queue doesn't point to the
right queue, so sch_htb can lose track of used netdev queues, which will
cause internal inconsistencies.
This commit fixes this bug by keeping track of the netdev queue inside
struct htb_class. All reads of cl->leaf.q->dev_queue are replaced by the
new field, the two values are synced on writes, and WARNs are added to
assert equality of the two values.
The driver API has changed: when TC_HTB_LEAF_DEL needs to move a queue,
the driver used to pass the old and new queue IDs to sch_htb. Now that
there is a new field (offload_queue) in struct htb_class that needs to
be updated on this operation, the driver will pass the old class ID to
sch_htb instead (it already knows the new class ID).
Fixes: d03b195b5aa0 ("sch_htb: Hierarchical QoS hardware offload")
Signed-off-by: Maxim Mikityanskiy <maximmi@nvidia.com>
Reviewed-by: Tariq Toukan <tariqt@nvidia.com>
---
.../net/ethernet/mellanox/mlx5/core/en/qos.c | 15 ++-
.../net/ethernet/mellanox/mlx5/core/en/qos.h | 4 +-
.../net/ethernet/mellanox/mlx5/core/en_main.c | 3 +-
include/net/pkt_cls.h | 3 +-
net/sched/sch_htb.c | 97 ++++++++++++-------
5 files changed, 72 insertions(+), 50 deletions(-)
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en/qos.c b/drivers/net/ethernet/mellanox/mlx5/core/en/qos.c
index 5efe3278b0f6..1fd8baf19829 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en/qos.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en/qos.c
@@ -733,8 +733,8 @@ static void mlx5e_reset_qdisc(struct net_device *dev, u16 qid)
spin_unlock_bh(qdisc_lock(qdisc));
}
-int mlx5e_htb_leaf_del(struct mlx5e_priv *priv, u16 classid, u16 *old_qid,
- u16 *new_qid, struct netlink_ext_ack *extack)
+int mlx5e_htb_leaf_del(struct mlx5e_priv *priv, u16 *classid,
+ struct netlink_ext_ack *extack)
{
struct mlx5e_qos_node *node;
struct netdev_queue *txq;
@@ -742,11 +742,9 @@ int mlx5e_htb_leaf_del(struct mlx5e_priv *priv, u16 classid, u16 *old_qid,
bool opened;
int err;
- qos_dbg(priv->mdev, "TC_HTB_LEAF_DEL classid %04x\n", classid);
-
- *old_qid = *new_qid = 0;
+ qos_dbg(priv->mdev, "TC_HTB_LEAF_DEL classid %04x\n", *classid);
- node = mlx5e_sw_node_find(priv, classid);
+ node = mlx5e_sw_node_find(priv, *classid);
if (!node)
return -ENOENT;
@@ -764,7 +762,7 @@ int mlx5e_htb_leaf_del(struct mlx5e_priv *priv, u16 classid, u16 *old_qid,
err = mlx5_qos_destroy_node(priv->mdev, node->hw_id);
if (err) /* Not fatal. */
qos_warn(priv->mdev, "Failed to destroy leaf node %u (class %04x), err = %d\n",
- node->hw_id, classid, err);
+ node->hw_id, *classid, err);
mlx5e_sw_node_delete(priv, node);
@@ -826,8 +824,7 @@ int mlx5e_htb_leaf_del(struct mlx5e_priv *priv, u16 classid, u16 *old_qid,
if (opened)
mlx5e_reactivate_qos_sq(priv, moved_qid, txq);
- *old_qid = mlx5e_qid_from_qos(&priv->channels, moved_qid);
- *new_qid = mlx5e_qid_from_qos(&priv->channels, qid);
+ *classid = node->classid;
return 0;
}
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en/qos.h b/drivers/net/ethernet/mellanox/mlx5/core/en/qos.h
index 5af7991fcd19..757682b7c0e0 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en/qos.h
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en/qos.h
@@ -34,8 +34,8 @@ int mlx5e_htb_leaf_alloc_queue(struct mlx5e_priv *priv, u16 classid,
struct netlink_ext_ack *extack);
int mlx5e_htb_leaf_to_inner(struct mlx5e_priv *priv, u16 classid, u16 child_classid,
u64 rate, u64 ceil, struct netlink_ext_ack *extack);
-int mlx5e_htb_leaf_del(struct mlx5e_priv *priv, u16 classid, u16 *old_qid,
- u16 *new_qid, struct netlink_ext_ack *extack);
+int mlx5e_htb_leaf_del(struct mlx5e_priv *priv, u16 *classid,
+ struct netlink_ext_ack *extack);
int mlx5e_htb_leaf_del_last(struct mlx5e_priv *priv, u16 classid, bool force,
struct netlink_ext_ack *extack);
int mlx5e_htb_node_modify(struct mlx5e_priv *priv, u16 classid, u64 rate, u64 ceil,
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
index 24f919ef9b8e..1a111af6ef7f 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
@@ -3445,8 +3445,7 @@ static int mlx5e_setup_tc_htb(struct mlx5e_priv *priv, struct tc_htb_qopt_offloa
return mlx5e_htb_leaf_to_inner(priv, htb->parent_classid, htb->classid,
htb->rate, htb->ceil, htb->extack);
case TC_HTB_LEAF_DEL:
- return mlx5e_htb_leaf_del(priv, htb->classid, &htb->moved_qid, &htb->qid,
- htb->extack);
+ return mlx5e_htb_leaf_del(priv, &htb->classid, htb->extack);
case TC_HTB_LEAF_DEL_LAST:
case TC_HTB_LEAF_DEL_LAST_FORCE:
return mlx5e_htb_leaf_del_last(priv, htb->classid,
diff --git a/include/net/pkt_cls.h b/include/net/pkt_cls.h
index 298a8d10168b..fb34b66aefa7 100644
--- a/include/net/pkt_cls.h
+++ b/include/net/pkt_cls.h
@@ -824,10 +824,9 @@ enum tc_htb_command {
struct tc_htb_qopt_offload {
struct netlink_ext_ack *extack;
enum tc_htb_command command;
- u16 classid;
u32 parent_classid;
+ u16 classid;
u16 qid;
- u16 moved_qid;
u64 rate;
u64 ceil;
};
diff --git a/net/sched/sch_htb.c b/net/sched/sch_htb.c
index 5f7ac27a5264..f22d26a2c89f 100644
--- a/net/sched/sch_htb.c
+++ b/net/sched/sch_htb.c
@@ -125,6 +125,7 @@ struct htb_class {
struct htb_class_leaf {
int deficit[TC_HTB_MAXDEPTH];
struct Qdisc *q;
+ struct netdev_queue *offload_queue;
} leaf;
struct htb_class_inner {
struct htb_prio clprio[TC_HTB_NUMPRIO];
@@ -1411,24 +1412,47 @@ htb_graft_helper(struct netdev_queue *dev_queue, struct Qdisc *new_q)
return old_q;
}
-static void htb_offload_move_qdisc(struct Qdisc *sch, u16 qid_old, u16 qid_new)
+static struct netdev_queue *htb_offload_get_queue(struct htb_class *cl)
+{
+ struct netdev_queue *queue;
+
+ queue = cl->leaf.offload_queue;
+ if (!(cl->leaf.q->flags & TCQ_F_BUILTIN))
+ WARN_ON(cl->leaf.q->dev_queue != queue);
+
+ return queue;
+}
+
+static void htb_offload_move_qdisc(struct Qdisc *sch, struct htb_class *cl_old,
+ struct htb_class *cl_new, bool destroying)
{
struct netdev_queue *queue_old, *queue_new;
struct net_device *dev = qdisc_dev(sch);
- struct Qdisc *qdisc;
- queue_old = netdev_get_tx_queue(dev, qid_old);
- queue_new = netdev_get_tx_queue(dev, qid_new);
+ queue_old = htb_offload_get_queue(cl_old);
+ queue_new = htb_offload_get_queue(cl_new);
- if (dev->flags & IFF_UP)
- dev_deactivate(dev);
- qdisc = dev_graft_qdisc(queue_old, NULL);
- qdisc->dev_queue = queue_new;
- qdisc = dev_graft_qdisc(queue_new, qdisc);
- if (dev->flags & IFF_UP)
- dev_activate(dev);
+ if (!destroying) {
+ struct Qdisc *qdisc;
- WARN_ON(!(qdisc->flags & TCQ_F_BUILTIN));
+ if (dev->flags & IFF_UP)
+ dev_deactivate(dev);
+ qdisc = dev_graft_qdisc(queue_old, NULL);
+ WARN_ON(qdisc != cl_old->leaf.q);
+ }
+
+ if (!(cl_old->leaf.q->flags & TCQ_F_BUILTIN))
+ cl_old->leaf.q->dev_queue = queue_new;
+ cl_old->leaf.offload_queue = queue_new;
+
+ if (!destroying) {
+ struct Qdisc *qdisc;
+
+ qdisc = dev_graft_qdisc(queue_new, cl_old->leaf.q);
+ if (dev->flags & IFF_UP)
+ dev_activate(dev);
+ WARN_ON(!(qdisc->flags & TCQ_F_BUILTIN));
+ }
}
static int htb_graft(struct Qdisc *sch, unsigned long arg, struct Qdisc *new,
@@ -1442,10 +1466,8 @@ static int htb_graft(struct Qdisc *sch, unsigned long arg, struct Qdisc *new,
if (cl->level)
return -EINVAL;
- if (q->offload) {
- dev_queue = new->dev_queue;
- WARN_ON(dev_queue != cl->leaf.q->dev_queue);
- }
+ if (q->offload)
+ dev_queue = htb_offload_get_queue(cl);
if (!new) {
new = qdisc_create_dflt(dev_queue, &pfifo_qdisc_ops,
@@ -1514,6 +1536,8 @@ static void htb_parent_to_leaf(struct Qdisc *sch, struct htb_class *cl,
parent->ctokens = parent->cbuffer;
parent->t_c = ktime_get_ns();
parent->cmode = HTB_CAN_SEND;
+ if (q->offload)
+ parent->leaf.offload_queue = cl->leaf.offload_queue;
}
static void htb_parent_to_leaf_offload(struct Qdisc *sch,
@@ -1534,6 +1558,7 @@ static int htb_destroy_class_offload(struct Qdisc *sch, struct htb_class *cl,
struct netlink_ext_ack *extack)
{
struct tc_htb_qopt_offload offload_opt;
+ struct netdev_queue *dev_queue;
struct Qdisc *q = cl->leaf.q;
struct Qdisc *old = NULL;
int err;
@@ -1542,16 +1567,15 @@ static int htb_destroy_class_offload(struct Qdisc *sch, struct htb_class *cl,
return -EINVAL;
WARN_ON(!q);
- if (!destroying) {
- /* On destroy of HTB, two cases are possible:
- * 1. q is a normal qdisc, but q->dev_queue has noop qdisc.
- * 2. q is a noop qdisc (for nodes that were inner),
- * q->dev_queue is noop_netdev_queue.
+ dev_queue = htb_offload_get_queue(cl);
+ old = htb_graft_helper(dev_queue, NULL);
+ if (destroying)
+ /* Before HTB is destroyed, the kernel grafts noop_qdisc to
+ * all queues.
*/
- old = htb_graft_helper(q->dev_queue, NULL);
- WARN_ON(!old);
+ WARN_ON(!(old->flags & TCQ_F_BUILTIN));
+ else
WARN_ON(old != q);
- }
if (cl->parent) {
cl->parent->bstats_bias.bytes += q->bstats.bytes;
@@ -1570,18 +1594,17 @@ static int htb_destroy_class_offload(struct Qdisc *sch, struct htb_class *cl,
if (!err || destroying)
qdisc_put(old);
else
- htb_graft_helper(q->dev_queue, old);
+ htb_graft_helper(dev_queue, old);
if (last_child)
return err;
- if (!err && offload_opt.moved_qid != 0) {
- if (destroying)
- q->dev_queue = netdev_get_tx_queue(qdisc_dev(sch),
- offload_opt.qid);
- else
- htb_offload_move_qdisc(sch, offload_opt.moved_qid,
- offload_opt.qid);
+ if (!err && offload_opt.classid != TC_H_MIN(cl->common.classid)) {
+ u32 classid = TC_H_MAJ(sch->handle) |
+ TC_H_MIN(offload_opt.classid);
+ struct htb_class *moved_cl = htb_find(classid, sch);
+
+ htb_offload_move_qdisc(sch, moved_cl, cl, destroying);
}
return err;
@@ -1704,9 +1727,11 @@ static int htb_delete(struct Qdisc *sch, unsigned long arg,
}
if (last_child) {
- struct netdev_queue *dev_queue;
+ struct netdev_queue *dev_queue = sch->dev_queue;
+
+ if (q->offload)
+ dev_queue = htb_offload_get_queue(cl);
- dev_queue = q->offload ? cl->leaf.q->dev_queue : sch->dev_queue;
new_q = qdisc_create_dflt(dev_queue, &pfifo_qdisc_ops,
cl->parent->common.classid,
NULL);
@@ -1878,7 +1903,7 @@ static int htb_change_class(struct Qdisc *sch, u32 classid,
}
dev_queue = netdev_get_tx_queue(dev, offload_opt.qid);
} else { /* First child. */
- dev_queue = parent->leaf.q->dev_queue;
+ dev_queue = htb_offload_get_queue(parent);
old_q = htb_graft_helper(dev_queue, NULL);
WARN_ON(old_q != parent->leaf.q);
offload_opt = (struct tc_htb_qopt_offload) {
@@ -1935,6 +1960,8 @@ static int htb_change_class(struct Qdisc *sch, u32 classid,
/* leaf (we) needs elementary qdisc */
cl->leaf.q = new_q ? new_q : &noop_qdisc;
+ if (q->offload)
+ cl->leaf.offload_queue = dev_queue;
cl->parent = parent;
--
2.25.1
^ permalink raw reply related [flat|nested] 5+ messages in thread
end of thread, other threads:[~2021-08-31 13:52 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-08-31 13:52 [PATCH net] sch_htb: Fix inconsistency when leaf qdisc creation fails kernel test robot
-- strict thread matches above, loose matches on Subject: below --
2021-08-26 11:54 Maxim Mikityanskiy
2021-08-26 16:10 ` Eric Dumazet
2021-08-26 17:42 ` Saeed Mahameed
2021-08-30 23:50 ` patchwork-bot+netdevbpf
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.