Received-SPF: pass (google.com: domain of linux-kernel+bounces-47923-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Date: Thu, 1 Feb 2024 18:00:54 +0800
From: Dawei Li <dawei.li@shingroup.cn>
To: Rob Herring <robh@kernel.org>
Cc: frowand.list@gmail.com, devicetree@vger.kernel.org,
	linux-kernel@vger.kernel.org, set_pte_at@outlook.com
Subject: Re: [PATCH 1/2] of: Introduce __of_phandle_update_cache
Message-ID: <F096F87333105368+Zbtr1h1ryCvzA3fB@centos8>
References: <20240130105236.3097126-1-dawei.li@shingroup.cn>
 <20240130105236.3097126-2-dawei.li@shingroup.cn>
 <20240131212938.GB2303754-robh@kernel.org>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240131212938.GB2303754-robh@kernel.org>
Feedback-ID: bizesmtp:shingroup.cn:qybglogicsvrgz:qybglogicsvrgz5a-1

Hi Rob,

Thanks for reviewing,

On Wed, Jan 31, 2024 at 03:29:38PM -0600, Rob Herring wrote:
> 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. 

I read the history changelog and get that a _lot_ of lookup has been
taken before of_core_init(), so the update of cache in lookup operation
mean a lot to performance improvement.

> 
> > 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.

Agreed.

> 
> > 
> > Signed-off-by: Dawei Li <dawei.li@shingroup.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.

Yep, assertion under condition is odd.

> 
> > +
> > +	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 

Yes, the updating policy of dynamic nodes is exactly same with static nodes
(the ones in of_core_init()), no eviction/invalidation on _existing_ cache
involved.

> sense. I would imagine old entries are less likely to be accessed than 

Well, I don't think we are gonna implement a full-fledged cache replacing
algorithm such as LRU.

> 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.

It's kinda awkward that no such usecases/stats are available for now.

My motivation is simple as that:
As long as detached nodes are supposed to be removed from cache entries,
the newly inserted nodes should be added to cache entries, it is more
balanced and symmetric.

And I am sorry if it breaks/undermines current design.

Thanks,

    Dawei

> 
> Rob
>