Re: [PATCH net-next v2 14/14] selftests: dpll: add reference leak tests

[Ivan Vecera <ivecera@redhat.com>]

On 11/5/26 16:18, Petr Oros wrote:
> With CONFIG_DPLL_REFCNT_TRACKER the DPLL core keeps the stack traces of
> unreleased device and pin references in debugfs. The entries do not
> name their owner, so add library helpers that snapshot the entries
> present before a test creates its devices and fail when an entry that
> is not in the snapshot still holds references afterwards. An entry in
> the snapshot belongs to an object of another driver and is ignored
> even when its content changed, since other drivers take and drop
> references while the tests run. A concurrent dump holds references for
> a moment, so the check waits for the candidates to drain. Attributing
> by name means that a leak of an earlier run goes unnoticed and that an
> object reusing the address of a freed one from the snapshot can hide a
> leak, which is accepted, the entries carry nothing better to go by. A
> second run alongside would have its devices attributed to this one,
> which is one more reason for the lock every script takes.
> 
> Whether the tracker works is probed with a throwaway device, which has
> to show entries while it is live and to drain them once it is deleted,
> so that a kernel without the tracker is told apart from a broken
> simulator and from a leak. The probe runs as the first test of the
> stress script, which gains a final leak check, and of the new
> dpll_reftrack.sh, which registers a device, removes it and verifies
> that no reference survived. The snapshot file lives next to the lock
> file under /run, for the same reason the lock file does not live in
> /tmp. The library mounts debugfs when nothing else did, like it does
> for configfs, and the selftest config enables the tracker together
> with CONFIG_DEBUG_KERNEL, which it depends on, and debugfs.
> 
> Signed-off-by: Petr Oros <poros@redhat.com>

Which dump holds a reference here?

The references to a dpll_device or a dpll_pin are taken in
dpll_device_get(), dpll_device_register(), dpll_pin_get(),
fwnode_dpll_pin_find() and by the pin registration, in
dpll_xa_ref_pin_add() and dpll_xa_ref_dpll_add().

The netlink dump and doit handlers in drivers/dpll/dpll_netlink.c run
under dpll_lock and do not take one.

Could the sentence name what actually holds the references the check waits
for, or be dropped if nothing does? The same sentence is in the comment
above dpll_ref_tracker_check_leaks():

[...]

> diff --git a/tools/testing/selftests/drivers/net/lib/sh/dpll_lib.sh b/tools/testing/selftests/drivers/net/lib/sh/dpll_lib.sh
> index ada10ee4394fd5..d851237bdac440 100644
> --- a/tools/testing/selftests/drivers/net/lib/sh/dpll_lib.sh
> +++ b/tools/testing/selftests/drivers/net/lib/sh/dpll_lib.sh

[...]

> @@ -622,3 +639,212 @@ dpllsim_setup_standard_device()

[...]
> +# $1: snapshot file written by dpll_ref_tracker_snapshot before the
> +# objects under test were created. Return 0 when no reference to an
> +# object created since the snapshot is held any more. A concurrent dump
> +# holds references for a moment, so wait for the candidates to drain. A
> +# timeout means the references never drained, the candidates seen by the
> +# last check are then printed and count as leaked.
> +dpll_ref_tracker_check_leaks()
[...]

Thanks,
Ivan