Tc dplane conversion

[donaldsharp]

No description provided.

[greptile-apps]

Greptile Summary

This PR migrates TC qdisc kernel message handling from the main netlink thread to the dataplane thread, following the same pattern already used for FDB and neighbor reads. The startup cleanup of leftover zebra-owned qdiscs is decoupled into a master-thread policy handler (zebra_tc_qdisc_handle_notify) fed by a new DPLANE_OP_TC_QDISC_NOTIFY context.

• RTNLGRP_TC is moved from the main netlink groups to dplane_groups, and RTM_*QDISC/TCLASS/TFILTER messages are routed to dplane_netlink_information_fetch; netlink_qdisc_change now enqueues a generic notify ctx instead of acting directly.
• TC_QDISC_MAJOR_ZEBRA is promoted from a tc_netlink.c-private define to a public constant in zebra_tc.h so both the decoder and the policy handler share the same value.
• zebra_dplane_startup_stage is simplified to accept ns_id_t directly, and the topotest replaces time.sleep(3) with run_and_expect polling plus a new startup-cleanup test case.

Confidence Score: 4/5

The architectural shift is clean and consistent with existing dplane patterns; the two nits are both in the error path of the startup TC read and do not affect the startup sequence completing correctly.

The core logic — moving TC processing to the dplane thread, decoupling policy into the master thread, and the startup cleanup — is sound. The only issues are that kernel_read_tc_qdisc unconditionally stamps the ctx SUCCESS even when netlink_request_qdiscs returns an error, and a copy-paste comment mislabels DPLANE_OP_TC_QDISC_NOTIFY as a read. Neither affects runtime behavior since the op-code result is not inspected by callers, but the misleading status could confuse future debugging.

zebra/tc_netlink.c (status handling in kernel_read_tc_qdisc) and zebra/zebra_dplane.h (wrong comment on DPLANE_OP_TC_QDISC_NOTIFY)

Important Files Changed

Filename	Overview
zebra/zebra_dplane.c	Adds DPLANE_OP_TC_QDISC_READ/NOTIFY ops, dplane_tc_qdisc_notify_info struct, accessor functions, enqueue helpers, log detail cases, and the kernel dplane processing path for TC qdisc reads. Changes zebra_dplane_startup_stage signature to take ns_id_t directly.
zebra/zebra_dplane.h	Declares new op codes, dplane_tc_qdisc_notify_e enum, accessor/enqueue prototypes, and updates zebra_dplane_startup_stage to take ns_id_t. Minor: duplicate wrong comment for DPLANE_OP_TC_QDISC_NOTIFY.
zebra/tc_netlink.c	Moves TC message handling to dplane thread: kernel_read_tc_qdisc now takes a dplane ctx and uses the dplane nlsock; netlink_qdisc_change enqueues a DPLANE_OP_TC_QDISC_NOTIFY instead of acting directly. netlink_request_qdiscs failure doesn't update ctx status to FAILURE.
zebra/kernel_netlink.c	Moves RTNLGRP_TC from main-thread groups to dplane_groups and routes RTM_*QDISC/TCLASS/TFILTER to dplane_netlink_information_fetch. Consistent with the architectural shift.
zebra/zebra_tc.c	Adds zebra_tc_qdisc_handle_notify() — the master-thread policy handler that removes leftover zebra-owned qdiscs seen during startup.
zebra/zebra_tc.h	Promotes TC_QDISC_MAJOR_ZEBRA to a public header constant shared by both the dplane decoder and the master-thread policy handler.
zebra/zebra_ns.c	Replaces direct kernel_read_tc_qdisc(zns) with dplane_tc_qdisc_read(zns) so TC startup read goes through the dplane pipeline.
zebra/rtread_netlink.c	Removes the old kernel_read_tc_qdisc implementation (superseded by the dplane-aware version in tc_netlink.c).
zebra/rtread_sysctl.c	Updates non-Linux stub kernel_read_tc_qdisc to the new dplane ctx signature and signals ZEBRA_DPLANE_FINISHED_READING via the ctx ns_id.
zebra/zebra_rib.c	Dispatches DPLANE_OP_TC_QDISC_NOTIFY to zebra_tc_qdisc_handle_notify; DPLANE_OP_TC_QDISC_READ is consumed silently.
tests/topotests/tc_basic/test_tc_basic.py	Replaces fragile time.sleep(3) with run_and_expect polling and adds test_tc_startup_removes_leftover_qdisc covering the new startup cleanup path.

Prompt To Fix All With AI

Fix the following 2 code review issues. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 2
zebra/zebra_dplane.h:228-229
The comment for `DPLANE_OP_TC_QDISC_NOTIFY` is copy-pasted from the one above and says "read" instead of "notification". It describes the wrong operation.

```suggestion
	/* Traffic control qdisc kernel notification */
	DPLANE_OP_TC_QDISC_NOTIFY,
```

### Issue 2 of 2
zebra/tc_netlink.c:823-827
When `netlink_request_qdiscs` returns an error the context status is still forced to `SUCCESS`. The failure path that already handles the nlsock-lookup error (above) correctly sets `FAILURE`; the same should happen here so the ctx status accurately reflects whether the kernel dump was requested successfully.

```suggestion
	ret = netlink_request_qdiscs(nl, AF_UNSPEC, RTM_GETQDISC);
	if (ret >= 0)
		netlink_parse_info(netlink_qdisc_change, nl, dp_info, 0, true);

	dplane_ctx_set_status(ctx, (ret >= 0) ? ZEBRA_DPLANE_REQUEST_SUCCESS
					      : ZEBRA_DPLANE_REQUEST_FAILURE);
```

_{Reviews (1): Last reviewed commit: "tests: Test startup of tc with tc_basic" | Re-trigger Greptile}

[greptile-apps]

The comment for DPLANE_OP_TC_QDISC_NOTIFY is copy-pasted from the one above and says "read" instead of "notification". It describes the wrong operation.

Suggested change

	/* Traffic control qdisc read */
	DPLANE_OP_TC_QDISC_NOTIFY,
	/* Traffic control qdisc kernel notification */
	DPLANE_OP_TC_QDISC_NOTIFY,

Prompt To Fix With AI

This is a comment left during a code review.
Path: zebra/zebra_dplane.h
Line: 228-229

Comment:
The comment for `DPLANE_OP_TC_QDISC_NOTIFY` is copy-pasted from the one above and says "read" instead of "notification". It describes the wrong operation.

```suggestion
	/* Traffic control qdisc kernel notification */
	DPLANE_OP_TC_QDISC_NOTIFY,
```

How can I resolve this? If you propose a fix, please make it concise.

[greptile-apps]

When netlink_request_qdiscs returns an error the context status is still forced to SUCCESS. The failure path that already handles the nlsock-lookup error (above) correctly sets FAILURE; the same should happen here so the ctx status accurately reflects whether the kernel dump was requested successfully.

Suggested change

	ret = netlink_request_qdiscs(nl, AF_UNSPEC, RTM_GETQDISC);
	if (ret >= 0)
	netlink_parse_info(netlink_qdisc_change, nl, dp_info, 0, true);
	ret = netlink_parse_info(netlink_qdisc_change, &zns->netlink_cmd,
	&dp_info, 0, true);
	if (ret < 0)
	return ret;
	dplane_ctx_set_status(ctx, ZEBRA_DPLANE_REQUEST_SUCCESS);
	ret = netlink_request_qdiscs(nl, AF_UNSPEC, RTM_GETQDISC);
	if (ret >= 0)
	netlink_parse_info(netlink_qdisc_change, nl, dp_info, 0, true);
	dplane_ctx_set_status(ctx, (ret >= 0) ? ZEBRA_DPLANE_REQUEST_SUCCESS
	: ZEBRA_DPLANE_REQUEST_FAILURE);

Prompt To Fix With AI

This is a comment left during a code review.
Path: zebra/tc_netlink.c
Line: 823-827

Comment:
When `netlink_request_qdiscs` returns an error the context status is still forced to `SUCCESS`. The failure path that already handles the nlsock-lookup error (above) correctly sets `FAILURE`; the same should happen here so the ctx status accurately reflects whether the kernel dump was requested successfully.

```suggestion
	ret = netlink_request_qdiscs(nl, AF_UNSPEC, RTM_GETQDISC);
	if (ret >= 0)
		netlink_parse_info(netlink_qdisc_change, nl, dp_info, 0, true);

	dplane_ctx_set_status(ctx, (ret >= 0) ? ZEBRA_DPLANE_REQUEST_SUCCESS
					      : ZEBRA_DPLANE_REQUEST_FAILURE);
```

How can I resolve this? If you propose a fix, please make it concise.

[mjstapp]

just had one small nit about the new enum value.
a little broader question though: it seems a bit off that the small TC module would be the place that makes the global "READ FINISHED" decision. I'd have thought that the dplane might be the place where that decision got made - each category would do its read, their individual FINISHED events would pass back through the dplane, and it would then decide when all the required categories had been done.

[mjstapp]

cut-and-paste comment?

[donaldsharp]

If you put the finished on the side of the dplane ( or really master zebra pthread ), there is no way to know when something is truly finished then. If we ever get to a situation where we can actually query something besides the linux kernel( or *bsd ), then we want to be able to have that signal come up through the dplane. I put it in the deep in the dplane in hopes that the final developer of this would notice that the signal was not happening and would then send it upon investigation.