lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20240131212938.GB2303754-robh@kernel.org>
Date: Wed, 31 Jan 2024 15:29:38 -0600
From: Rob Herring <robh@...nel.org>
To: Dawei Li <dawei.li@...ngroup.cn>
Cc: frowand.list@...il.com, devicetree@...r.kernel.org,
	linux-kernel@...r.kernel.org, set_pte_at@...look.com
Subject: Re: [PATCH 1/2] of: Introduce __of_phandle_update_cache

On Tue, Jan 30, 2024 at 06:52:35PM +0800, Dawei Li wrote:
> For system with CONFIG_OF_DYNAMIC=y, device nodes can be inserted/removed
> dynamically from device tree. Meanwhile phandle_cache is created for fast
> lookup from phandle to device node.

Why do we need it to be fast? What's the usecase (upstream dynamic DT 
usecases are limited) and what's the performance difference? We'll 
already cache the new phandle on the first lookup. Plus with only 128 
entries you are likely evicting an entry. 

> For node detach, phandle cache of removed node is invalidated to maintain
> the mapping up to date, but the counterpart operation on node attach is
> not implemented yet.
> 
> Thus, implement the cache updating operation on node attach.

Except this patch does not do that. The next patch does.

> 
> Signed-off-by: Dawei Li <dawei.li@...ngroup.cn>
> ---
>  drivers/of/base.c       | 16 ++++++++++++++++
>  drivers/of/of_private.h |  1 +
>  2 files changed, 17 insertions(+)
> 
> diff --git a/drivers/of/base.c b/drivers/of/base.c
> index b0ad8fc06e80..8b7da27835eb 100644
> --- a/drivers/of/base.c
> +++ b/drivers/of/base.c
> @@ -163,6 +163,22 @@ void __of_phandle_cache_inv_entry(phandle handle)
>  		phandle_cache[handle_hash] = NULL;
>  }
>  
> +void __of_phandle_update_cache(struct device_node *np, bool lock)
> +{
> +	u32 hash;
> +
> +	if (lock)
> +		lockdep_assert_held(&devtree_lock);

I don't think this is a good use of a function parameter.

> +
> +	if (unlikely(!np || !np->phandle))
> +		return;
> +
> +	hash = of_phandle_cache_hash(np->phandle);
> +
> +	if (!phandle_cache[hash])
> +		phandle_cache[hash] = np;

Okay, so you don't evict existing entries. I'm not sure what makes more 
sense. I would imagine old entries are less likely to be accessed than 
new phandles for just added nodes given DT is kind of parse it all once 
(e.g. at boot time). Again, need to understand your usecase and 
performance differences.

Rob

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ