Re: [PATCH] Revert "fdtdec: drop needlessly convoluted CONFIG_PHANDLE_CHECK_SEQ"
Hi Rasmus,
On Tue, 18 Apr 2023 at 23:30, Rasmus Villemoes
wrote:
>
> On 19/04/2023 03.49, Simon Glass wrote:
> > Hi Rasmus,
> >
> > Returning to this old thread...
> >
> >>
> >> There's no problematic device tree, having two nodes with the same
> >> (base)name is perfectly fine and in fact sometimes expected/required (in
> >> the cases where a node name is standardized; e.g. having two identical
> >> eeproms on different i2c buses both would/should be called eeprom@50).
> >>
> >> What Simon is concerned about is that the translation from full path to
> >> blob offset is now done unconditionally, and that might be costly. I'm
> >> not sure it is (and didn't know that it could be), but as I've said
> >> repeatedly, I prefer code that works out-of-the-box, and for the boards
> >> that do need this extra check (because just looking at the basename is
> >> not enough), the fact that the previous code worked seemed to be pure
> >> luck - because those dtbs were compiled with -@ due to some other
> >> CONFIG_ knob being set. And again, involving phandles at all is what
> >> make the code fragile, so a revert that reinstates a CONFIG_ knob with
> >> PHANDLE in the name is not a good way forward, assuming there is even
> >> anything to fix.
> >>
> >> So if the performance thing is real, sure, we can introduce a
> >> CONFIG_something, but only if at the same time we have a sanity check at
> >> build-time that detects if one should enable/disable that option
> >> (depending on whether we make it "positive" or "negative"). Something
> >> like this seems to do the trick, but I haven't looked at hooking it up
> >> in the build system:
> >>
> >> === scripts/check-alias-homonyms.py ===
> >> #!/usr/bin/python3
> >>
> >> import sys
> >>
> >> sys.path.insert(0, 'scripts/dtc/pylibfdt')
> >> sys.path.insert(1, 'tools')
> >>
> >> from dtoc import fdt
> >>
> >> ret = 0
> >>
> >> for name in sys.argv[1:]:
> >> dtb = fdt.FdtScan(name)
> >> aliasnode = dtb.GetNode("/aliases")
> >> if not aliasnode:
> >> continue
> >> basenames = dict()
> >> for prop in aliasnode.props.values():
> >> alias = prop.name
> >> path = prop.value
> >> base = path[path.rfind('/')+1:]
> >> if base in basenames:
> >> basenames[base].append(alias)
> >> else:
> >> basenames[base] = [alias]
> >> for base, aliases in basenames.items():
> >> if len(aliases) == 1:
> >> continue
> >> print("Warning: In %s, the aliases %s all point at nodes with
> >> the basename '%s' - consider (en/dis)abling CONFIG_..." % (name,
> >> ",".join(aliases), base))
> >> ret = 1
> >>
> >> sys.exit(ret)
> >> ===
> >>
> >> I imagine this being run over CONFIG_OF_LIST when CONFIG_... has the
> >> value that disables the fdt_path_offset() call. For concreteness,
> >> something like
> >>
> >> config ASSUME_ALIAS_HOMONYMS # or whatever better name one can come up with
> >> bool "Assume there may be nodes pointed to by aliases in DT that have
> >> identical basenames"
> >> help
> >> In most cases, the nodes pointed to by aliases in the device tree
> >> all have different basenames. When this is satisfied, the
> >> fdtdec_get_alias_seq() function can avoid a somewhat expensive full walk
> >> of the dtb when looking for an alias for a given node. If the device
> >> tree for your board does have aliases pointing at nodes with the same
> >> basenames (but of course different full paths), that full walk is
> >> necessary for correctness, and you can then enable this option.
> >>
> >> plus
> >>
> >> - if (offset != fdt_path_offset(blob, prop))
> >> + if (IS_ENABLED(CONFIG_ASSUME_ALIAS_HOMONYMS) && offset !=
> >> fdt_path_offset(blob, prop))
> >>
> >> I have no idea what running the above python script on the dtb(s) in the
> >> common case of !CONFIG_ASSUME_ALIAS_HOMONYMS adds to buildtime, but I do
> >> believe that before eliding some code that is necessary for correctness
> >> in the general case, we need buildtime verification that it's ok.
> >
> > The runtime performance impact is real. We actually have quite a
> > problem in general, e.g. the pinctrl drivers can take ages to operate
> > before relocation because they read the DT multiple times.
>
> OK. For other reasons, I'm looking at the upstream dtc code, and have
> some ideas for performance improvements to the "raw blob walking" code.
> But I don't have any idea how large an impact it could have, if it would
> help that pinctrl case, or when I'll actually find some spare cycles to
> work on it.
OK. Part of the issue with pinctrl might be that drivers should be
caching the DT in their internal structures, rather than scanning it
each time pinctrl_select_state() is called.
>
> > Your script seems like a great idea to me. Perhaps it should produce
> > an error if the CONFIG is not enabled?
>
> I was imagining that it would only run when the config is not enabled
> (because when it is enabled,
Re: [PATCH] Revert "fdtdec: drop needlessly convoluted CONFIG_PHANDLE_CHECK_SEQ"
On 19/04/2023 03.49, Simon Glass wrote:
> Hi Rasmus,
>
> Returning to this old thread...
>
>>
>> There's no problematic device tree, having two nodes with the same
>> (base)name is perfectly fine and in fact sometimes expected/required (in
>> the cases where a node name is standardized; e.g. having two identical
>> eeproms on different i2c buses both would/should be called eeprom@50).
>>
>> What Simon is concerned about is that the translation from full path to
>> blob offset is now done unconditionally, and that might be costly. I'm
>> not sure it is (and didn't know that it could be), but as I've said
>> repeatedly, I prefer code that works out-of-the-box, and for the boards
>> that do need this extra check (because just looking at the basename is
>> not enough), the fact that the previous code worked seemed to be pure
>> luck - because those dtbs were compiled with -@ due to some other
>> CONFIG_ knob being set. And again, involving phandles at all is what
>> make the code fragile, so a revert that reinstates a CONFIG_ knob with
>> PHANDLE in the name is not a good way forward, assuming there is even
>> anything to fix.
>>
>> So if the performance thing is real, sure, we can introduce a
>> CONFIG_something, but only if at the same time we have a sanity check at
>> build-time that detects if one should enable/disable that option
>> (depending on whether we make it "positive" or "negative"). Something
>> like this seems to do the trick, but I haven't looked at hooking it up
>> in the build system:
>>
>> === scripts/check-alias-homonyms.py ===
>> #!/usr/bin/python3
>>
>> import sys
>>
>> sys.path.insert(0, 'scripts/dtc/pylibfdt')
>> sys.path.insert(1, 'tools')
>>
>> from dtoc import fdt
>>
>> ret = 0
>>
>> for name in sys.argv[1:]:
>> dtb = fdt.FdtScan(name)
>> aliasnode = dtb.GetNode("/aliases")
>> if not aliasnode:
>> continue
>> basenames = dict()
>> for prop in aliasnode.props.values():
>> alias = prop.name
>> path = prop.value
>> base = path[path.rfind('/')+1:]
>> if base in basenames:
>> basenames[base].append(alias)
>> else:
>> basenames[base] = [alias]
>> for base, aliases in basenames.items():
>> if len(aliases) == 1:
>> continue
>> print("Warning: In %s, the aliases %s all point at nodes with
>> the basename '%s' - consider (en/dis)abling CONFIG_..." % (name,
>> ",".join(aliases), base))
>> ret = 1
>>
>> sys.exit(ret)
>> ===
>>
>> I imagine this being run over CONFIG_OF_LIST when CONFIG_... has the
>> value that disables the fdt_path_offset() call. For concreteness,
>> something like
>>
>> config ASSUME_ALIAS_HOMONYMS # or whatever better name one can come up with
>> bool "Assume there may be nodes pointed to by aliases in DT that have
>> identical basenames"
>> help
>> In most cases, the nodes pointed to by aliases in the device tree
>> all have different basenames. When this is satisfied, the
>> fdtdec_get_alias_seq() function can avoid a somewhat expensive full walk
>> of the dtb when looking for an alias for a given node. If the device
>> tree for your board does have aliases pointing at nodes with the same
>> basenames (but of course different full paths), that full walk is
>> necessary for correctness, and you can then enable this option.
>>
>> plus
>>
>> - if (offset != fdt_path_offset(blob, prop))
>> + if (IS_ENABLED(CONFIG_ASSUME_ALIAS_HOMONYMS) && offset !=
>> fdt_path_offset(blob, prop))
>>
>> I have no idea what running the above python script on the dtb(s) in the
>> common case of !CONFIG_ASSUME_ALIAS_HOMONYMS adds to buildtime, but I do
>> believe that before eliding some code that is necessary for correctness
>> in the general case, we need buildtime verification that it's ok.
>
> The runtime performance impact is real. We actually have quite a
> problem in general, e.g. the pinctrl drivers can take ages to operate
> before relocation because they read the DT multiple times.
OK. For other reasons, I'm looking at the upstream dtc code, and have
some ideas for performance improvements to the "raw blob walking" code.
But I don't have any idea how large an impact it could have, if it would
help that pinctrl case, or when I'll actually find some spare cycles to
work on it.
> Your script seems like a great idea to me. Perhaps it should produce
> an error if the CONFIG is not enabled?
I was imagining that it would only run when the config is not enabled
(because when it is enabled, correctness is guaranteed at runtime), and
AFAICT it already does produce an error when it finds homonyms. But it
really was just something I wrote in five minutes, I have no idea how to
hook it up in the build system or how to propagate that error. Anybody
is welcome to take that code and polish it up and make a real patch out
of it.
I also think I suggested doing something else at build time, something
along the lines of 'for each alias, inject a property in the
Re: [PATCH] Revert "fdtdec: drop needlessly convoluted CONFIG_PHANDLE_CHECK_SEQ"
Hi Rasmus,
On Mon, 1 Aug 2022 at 07:13, Rasmus Villemoes
wrote:
>
> On 31/07/2022 15.28, Tom Rini wrote:
> > On Sat, Jul 30, 2022 at 07:27:26PM -0600, Simon Glass wrote:
> >> Hi Tom,
> >>
>
> >> Shall I pick it up for the upcoming release?
> >
> > I don't think we should pick up the revert as I don't think there's
> > agreement that reverting this is the right step forward among all of the
> > interested parties.
> >
> > One thing I want to know at this point is, was the problematic device
> > tree accepted upstream, or did they also say that 2 nodes with the same
> > name but different paths is wrong, don't do that?
Returning to this old thread...
>
> There's no problematic device tree, having two nodes with the same
> (base)name is perfectly fine and in fact sometimes expected/required (in
> the cases where a node name is standardized; e.g. having two identical
> eeproms on different i2c buses both would/should be called eeprom@50).
>
> What Simon is concerned about is that the translation from full path to
> blob offset is now done unconditionally, and that might be costly. I'm
> not sure it is (and didn't know that it could be), but as I've said
> repeatedly, I prefer code that works out-of-the-box, and for the boards
> that do need this extra check (because just looking at the basename is
> not enough), the fact that the previous code worked seemed to be pure
> luck - because those dtbs were compiled with -@ due to some other
> CONFIG_ knob being set. And again, involving phandles at all is what
> make the code fragile, so a revert that reinstates a CONFIG_ knob with
> PHANDLE in the name is not a good way forward, assuming there is even
> anything to fix.
>
> So if the performance thing is real, sure, we can introduce a
> CONFIG_something, but only if at the same time we have a sanity check at
> build-time that detects if one should enable/disable that option
> (depending on whether we make it "positive" or "negative"). Something
> like this seems to do the trick, but I haven't looked at hooking it up
> in the build system:
>
> === scripts/check-alias-homonyms.py ===
> #!/usr/bin/python3
>
> import sys
>
> sys.path.insert(0, 'scripts/dtc/pylibfdt')
> sys.path.insert(1, 'tools')
>
> from dtoc import fdt
>
> ret = 0
>
> for name in sys.argv[1:]:
> dtb = fdt.FdtScan(name)
> aliasnode = dtb.GetNode("/aliases")
> if not aliasnode:
> continue
> basenames = dict()
> for prop in aliasnode.props.values():
> alias = prop.name
> path = prop.value
> base = path[path.rfind('/')+1:]
> if base in basenames:
> basenames[base].append(alias)
> else:
> basenames[base] = [alias]
> for base, aliases in basenames.items():
> if len(aliases) == 1:
> continue
> print("Warning: In %s, the aliases %s all point at nodes with
> the basename '%s' - consider (en/dis)abling CONFIG_..." % (name,
> ",".join(aliases), base))
> ret = 1
>
> sys.exit(ret)
> ===
>
> I imagine this being run over CONFIG_OF_LIST when CONFIG_... has the
> value that disables the fdt_path_offset() call. For concreteness,
> something like
>
> config ASSUME_ALIAS_HOMONYMS # or whatever better name one can come up with
> bool "Assume there may be nodes pointed to by aliases in DT that have
> identical basenames"
> help
> In most cases, the nodes pointed to by aliases in the device tree
> all have different basenames. When this is satisfied, the
> fdtdec_get_alias_seq() function can avoid a somewhat expensive full walk
> of the dtb when looking for an alias for a given node. If the device
> tree for your board does have aliases pointing at nodes with the same
> basenames (but of course different full paths), that full walk is
> necessary for correctness, and you can then enable this option.
>
> plus
>
> - if (offset != fdt_path_offset(blob, prop))
> + if (IS_ENABLED(CONFIG_ASSUME_ALIAS_HOMONYMS) && offset !=
> fdt_path_offset(blob, prop))
>
> I have no idea what running the above python script on the dtb(s) in the
> common case of !CONFIG_ASSUME_ALIAS_HOMONYMS adds to buildtime, but I do
> believe that before eliding some code that is necessary for correctness
> in the general case, we need buildtime verification that it's ok.
The runtime performance impact is real. We actually have quite a
problem in general, e.g. the pinctrl drivers can take ages to operate
before relocation because they read the DT multiple times.
Your script seems like a great idea to me. Perhaps it should produce
an error if the CONFIG is not enabled?
Regards,
Simon
Re: [PATCH] Revert "fdtdec: drop needlessly convoluted CONFIG_PHANDLE_CHECK_SEQ"
On 31/07/2022 15.28, Tom Rini wrote:
> On Sat, Jul 30, 2022 at 07:27:26PM -0600, Simon Glass wrote:
>> Hi Tom,
>>
>> Shall I pick it up for the upcoming release?
>
> I don't think we should pick up the revert as I don't think there's
> agreement that reverting this is the right step forward among all of the
> interested parties.
>
> One thing I want to know at this point is, was the problematic device
> tree accepted upstream, or did they also say that 2 nodes with the same
> name but different paths is wrong, don't do that?
There's no problematic device tree, having two nodes with the same
(base)name is perfectly fine and in fact sometimes expected/required (in
the cases where a node name is standardized; e.g. having two identical
eeproms on different i2c buses both would/should be called eeprom@50).
What Simon is concerned about is that the translation from full path to
blob offset is now done unconditionally, and that might be costly. I'm
not sure it is (and didn't know that it could be), but as I've said
repeatedly, I prefer code that works out-of-the-box, and for the boards
that do need this extra check (because just looking at the basename is
not enough), the fact that the previous code worked seemed to be pure
luck - because those dtbs were compiled with -@ due to some other
CONFIG_ knob being set. And again, involving phandles at all is what
make the code fragile, so a revert that reinstates a CONFIG_ knob with
PHANDLE in the name is not a good way forward, assuming there is even
anything to fix.
So if the performance thing is real, sure, we can introduce a
CONFIG_something, but only if at the same time we have a sanity check at
build-time that detects if one should enable/disable that option
(depending on whether we make it "positive" or "negative"). Something
like this seems to do the trick, but I haven't looked at hooking it up
in the build system:
=== scripts/check-alias-homonyms.py ===
#!/usr/bin/python3
import sys
sys.path.insert(0, 'scripts/dtc/pylibfdt')
sys.path.insert(1, 'tools')
from dtoc import fdt
ret = 0
for name in sys.argv[1:]:
dtb = fdt.FdtScan(name)
aliasnode = dtb.GetNode("/aliases")
if not aliasnode:
continue
basenames = dict()
for prop in aliasnode.props.values():
alias = prop.name
path = prop.value
base = path[path.rfind('/')+1:]
if base in basenames:
basenames[base].append(alias)
else:
basenames[base] = [alias]
for base, aliases in basenames.items():
if len(aliases) == 1:
continue
print("Warning: In %s, the aliases %s all point at nodes with
the basename '%s' - consider (en/dis)abling CONFIG_..." % (name,
",".join(aliases), base))
ret = 1
sys.exit(ret)
===
I imagine this being run over CONFIG_OF_LIST when CONFIG_... has the
value that disables the fdt_path_offset() call. For concreteness,
something like
config ASSUME_ALIAS_HOMONYMS # or whatever better name one can come up with
bool "Assume there may be nodes pointed to by aliases in DT that have
identical basenames"
help
In most cases, the nodes pointed to by aliases in the device tree
all have different basenames. When this is satisfied, the
fdtdec_get_alias_seq() function can avoid a somewhat expensive full walk
of the dtb when looking for an alias for a given node. If the device
tree for your board does have aliases pointing at nodes with the same
basenames (but of course different full paths), that full walk is
necessary for correctness, and you can then enable this option.
plus
- if (offset != fdt_path_offset(blob, prop))
+ if (IS_ENABLED(CONFIG_ASSUME_ALIAS_HOMONYMS) && offset !=
fdt_path_offset(blob, prop))
I have no idea what running the above python script on the dtb(s) in the
common case of !CONFIG_ASSUME_ALIAS_HOMONYMS adds to buildtime, but I do
believe that before eliding some code that is necessary for correctness
in the general case, we need buildtime verification that it's ok.
Rasmus
Re: [PATCH] Revert "fdtdec: drop needlessly convoluted CONFIG_PHANDLE_CHECK_SEQ"
On Sat, Jul 30, 2022 at 07:27:26PM -0600, Simon Glass wrote: > Hi Tom, > > On Tue, 5 Jul 2022 at 10:42, Simon Glass wrote: > > > > Hi Rasmus, > > > > On Tue, 5 Jul 2022 at 07:47, Rasmus Villemoes > > wrote: > > > > > > On 05/07/2022 11.47, Simon Glass wrote: > > > > Hi Tom, > > > > > > > > On Sun, 3 Jul 2022 at 06:43, Tom Rini wrote: > > > >> > > > >> On Sun, Jul 03, 2022 at 02:32:42AM -0600, Simon Glass wrote: > > > >>> Hi, > > > >>> > > > >>> On Sun, 3 Jul 2022 at 02:25, Simon Glass wrote: > > > > > > The fdt_path_offset() function is slow since it must scan the tree. > > > This substantial overhead now applies to all boards. > > > > > > The original code may not be ideal but it is fit for purpose and is > > > only > > > needed on a few boards. > > > > > > We should revert this in time for the release. > > > > > > This reverts commit 26f981f295d00351b6f0c69b5317b254b2361cc0. > > > > > > Signed-off-by: Simon Glass > > > --- > > > > > > configs/am65x_evm_a53_defconfig | 1 + > > > configs/evb-ast2600_defconfig| 1 + > > > configs/sama7g5ek_mmc1_defconfig | 1 + > > > configs/sama7g5ek_mmc_defconfig | 1 + > > > lib/Kconfig | 7 +++ > > > lib/fdtdec.c | 7 +-- > > > 6 files changed, 16 insertions(+), 2 deletions(-) > > > >>> > > > >>> Please also see the context here: > > > >>> > > > >>> https://patchwork.ozlabs.org/project/uboot/patch/2020142605.17034-1-a-govindr...@ti.com/ > > > >> > > > >> Previously when we've had issues of making the fast path slow, people > > > >> have posted measurements of before/after in order to demonstrate the > > > >> problem. Can we please get some logs of before/after and various > > > >> possible solutions? Thanks. > > > > > > > > Well this code is not needed at all for all but four boards. > > > > > > Three. One board seems to enable that config for no reason at all. And > > > it wouldn't work on that board, because the code was fragile and error > > > prone. See my detailed explanations in the original patch thread. > > > > > > It does a > > > > very expensive check of the DT and this can happen before relocation, > > > > or is SPL. I don't have a board to test with at present, but I expect > > > > it would cost 10s of milliseconds on AT91, for example. > > > > > > As I've said before, I prefer code which is correct out-of-the-box. I > > > also prefer simpler code with less ifdefs (yeah yeah, IS_ENABLED...) and > > > fewer configuration options to worry about. The three boards which have > > > aliases where the leaf nodes have duplicate names also happen to enable > > > some other unrelated magic config knob which happens to make the phandle > > > comparison work. So at the very least the code should stop comparing > > > phandles and just compare the node offsets; whether that check should be > > > > I don't disagree that this is a bit odd, but it is efficient. > > > > Another approach here would be to add better documentation since the > > option doesn't quite work as advertised. > > > > > under a config knob can be discussed (certainly that config knob should > > > not have PHANDLE in it), but, again, as I've said, it should be opt-out, > > > and preferably with a build-time check that verifies that no two aliases > > > point at same basenames. > > > > Opt-out means that everything pays the penalty, though. This is real > > corner case. Arguably the device tree should be updated to avoid this > > problem. > > > > > > > > _If_ that 10s of milliseconds figure is true, there are other things one > > > could do at build time. Say have some pass over the dtb which simply > > > adds "u-boot,seq" properties to the target nodes of aliases, using that > > > if present, or add a "back pointer" "u-boot,alias" property one could > > > compare to name. > > > > That's a nice idea. > > > > The point of my revert was to get something in for the release. I > > fully expect some sort of change to go in afterwards, but I don't > > think people were aware of the impact of your patch (see context link > > above). > > > > So I still favour a revert, for now. > > It looks like this didn't make it for the release. I'm not sure if > this is causing problems. > > Shall I pick it up for the upcoming release? I don't think we should pick up the revert as I don't think there's agreement that reverting this is the right step forward among all of the interested parties. One thing I want to know at this point is, was the problematic device tree accepted upstream, or did they also say that 2 nodes with the same name but different paths is wrong, don't do that? -- Tom signature.asc Description: PGP signature
Re: [PATCH] Revert "fdtdec: drop needlessly convoluted CONFIG_PHANDLE_CHECK_SEQ"
Hi Tom, On Tue, 5 Jul 2022 at 10:42, Simon Glass wrote: > > Hi Rasmus, > > On Tue, 5 Jul 2022 at 07:47, Rasmus Villemoes > wrote: > > > > On 05/07/2022 11.47, Simon Glass wrote: > > > Hi Tom, > > > > > > On Sun, 3 Jul 2022 at 06:43, Tom Rini wrote: > > >> > > >> On Sun, Jul 03, 2022 at 02:32:42AM -0600, Simon Glass wrote: > > >>> Hi, > > >>> > > >>> On Sun, 3 Jul 2022 at 02:25, Simon Glass wrote: > > > > The fdt_path_offset() function is slow since it must scan the tree. > > This substantial overhead now applies to all boards. > > > > The original code may not be ideal but it is fit for purpose and is > > only > > needed on a few boards. > > > > We should revert this in time for the release. > > > > This reverts commit 26f981f295d00351b6f0c69b5317b254b2361cc0. > > > > Signed-off-by: Simon Glass > > --- > > > > configs/am65x_evm_a53_defconfig | 1 + > > configs/evb-ast2600_defconfig| 1 + > > configs/sama7g5ek_mmc1_defconfig | 1 + > > configs/sama7g5ek_mmc_defconfig | 1 + > > lib/Kconfig | 7 +++ > > lib/fdtdec.c | 7 +-- > > 6 files changed, 16 insertions(+), 2 deletions(-) > > >>> > > >>> Please also see the context here: > > >>> > > >>> https://patchwork.ozlabs.org/project/uboot/patch/2020142605.17034-1-a-govindr...@ti.com/ > > >> > > >> Previously when we've had issues of making the fast path slow, people > > >> have posted measurements of before/after in order to demonstrate the > > >> problem. Can we please get some logs of before/after and various > > >> possible solutions? Thanks. > > > > > > Well this code is not needed at all for all but four boards. > > > > Three. One board seems to enable that config for no reason at all. And > > it wouldn't work on that board, because the code was fragile and error > > prone. See my detailed explanations in the original patch thread. > > > > It does a > > > very expensive check of the DT and this can happen before relocation, > > > or is SPL. I don't have a board to test with at present, but I expect > > > it would cost 10s of milliseconds on AT91, for example. > > > > As I've said before, I prefer code which is correct out-of-the-box. I > > also prefer simpler code with less ifdefs (yeah yeah, IS_ENABLED...) and > > fewer configuration options to worry about. The three boards which have > > aliases where the leaf nodes have duplicate names also happen to enable > > some other unrelated magic config knob which happens to make the phandle > > comparison work. So at the very least the code should stop comparing > > phandles and just compare the node offsets; whether that check should be > > I don't disagree that this is a bit odd, but it is efficient. > > Another approach here would be to add better documentation since the > option doesn't quite work as advertised. > > > under a config knob can be discussed (certainly that config knob should > > not have PHANDLE in it), but, again, as I've said, it should be opt-out, > > and preferably with a build-time check that verifies that no two aliases > > point at same basenames. > > Opt-out means that everything pays the penalty, though. This is real > corner case. Arguably the device tree should be updated to avoid this > problem. > > > > > _If_ that 10s of milliseconds figure is true, there are other things one > > could do at build time. Say have some pass over the dtb which simply > > adds "u-boot,seq" properties to the target nodes of aliases, using that > > if present, or add a "back pointer" "u-boot,alias" property one could > > compare to name. > > That's a nice idea. > > The point of my revert was to get something in for the release. I > fully expect some sort of change to go in afterwards, but I don't > think people were aware of the impact of your patch (see context link > above). > > So I still favour a revert, for now. It looks like this didn't make it for the release. I'm not sure if this is causing problems. Shall I pick it up for the upcoming release? Regards, Simon
Re: [PATCH] Revert "fdtdec: drop needlessly convoluted CONFIG_PHANDLE_CHECK_SEQ"
Hi Rasmus, On Tue, 5 Jul 2022 at 07:47, Rasmus Villemoes wrote: > > On 05/07/2022 11.47, Simon Glass wrote: > > Hi Tom, > > > > On Sun, 3 Jul 2022 at 06:43, Tom Rini wrote: > >> > >> On Sun, Jul 03, 2022 at 02:32:42AM -0600, Simon Glass wrote: > >>> Hi, > >>> > >>> On Sun, 3 Jul 2022 at 02:25, Simon Glass wrote: > > The fdt_path_offset() function is slow since it must scan the tree. > This substantial overhead now applies to all boards. > > The original code may not be ideal but it is fit for purpose and is only > needed on a few boards. > > We should revert this in time for the release. > > This reverts commit 26f981f295d00351b6f0c69b5317b254b2361cc0. > > Signed-off-by: Simon Glass > --- > > configs/am65x_evm_a53_defconfig | 1 + > configs/evb-ast2600_defconfig| 1 + > configs/sama7g5ek_mmc1_defconfig | 1 + > configs/sama7g5ek_mmc_defconfig | 1 + > lib/Kconfig | 7 +++ > lib/fdtdec.c | 7 +-- > 6 files changed, 16 insertions(+), 2 deletions(-) > >>> > >>> Please also see the context here: > >>> > >>> https://patchwork.ozlabs.org/project/uboot/patch/2020142605.17034-1-a-govindr...@ti.com/ > >> > >> Previously when we've had issues of making the fast path slow, people > >> have posted measurements of before/after in order to demonstrate the > >> problem. Can we please get some logs of before/after and various > >> possible solutions? Thanks. > > > > Well this code is not needed at all for all but four boards. > > Three. One board seems to enable that config for no reason at all. And > it wouldn't work on that board, because the code was fragile and error > prone. See my detailed explanations in the original patch thread. > > It does a > > very expensive check of the DT and this can happen before relocation, > > or is SPL. I don't have a board to test with at present, but I expect > > it would cost 10s of milliseconds on AT91, for example. > > As I've said before, I prefer code which is correct out-of-the-box. I > also prefer simpler code with less ifdefs (yeah yeah, IS_ENABLED...) and > fewer configuration options to worry about. The three boards which have > aliases where the leaf nodes have duplicate names also happen to enable > some other unrelated magic config knob which happens to make the phandle > comparison work. So at the very least the code should stop comparing > phandles and just compare the node offsets; whether that check should be I don't disagree that this is a bit odd, but it is efficient. Another approach here would be to add better documentation since the option doesn't quite work as advertised. > under a config knob can be discussed (certainly that config knob should > not have PHANDLE in it), but, again, as I've said, it should be opt-out, > and preferably with a build-time check that verifies that no two aliases > point at same basenames. Opt-out means that everything pays the penalty, though. This is real corner case. Arguably the device tree should be updated to avoid this problem. > > _If_ that 10s of milliseconds figure is true, there are other things one > could do at build time. Say have some pass over the dtb which simply > adds "u-boot,seq" properties to the target nodes of aliases, using that > if present, or add a "back pointer" "u-boot,alias" property one could > compare to name. That's a nice idea. The point of my revert was to get something in for the release. I fully expect some sort of change to go in afterwards, but I don't think people were aware of the impact of your patch (see context link above). So I still favour a revert, for now. Regards, Simon
Re: [PATCH] Revert "fdtdec: drop needlessly convoluted CONFIG_PHANDLE_CHECK_SEQ"
On 05/07/2022 11.47, Simon Glass wrote: > Hi Tom, > > On Sun, 3 Jul 2022 at 06:43, Tom Rini wrote: >> >> On Sun, Jul 03, 2022 at 02:32:42AM -0600, Simon Glass wrote: >>> Hi, >>> >>> On Sun, 3 Jul 2022 at 02:25, Simon Glass wrote: The fdt_path_offset() function is slow since it must scan the tree. This substantial overhead now applies to all boards. The original code may not be ideal but it is fit for purpose and is only needed on a few boards. We should revert this in time for the release. This reverts commit 26f981f295d00351b6f0c69b5317b254b2361cc0. Signed-off-by: Simon Glass --- configs/am65x_evm_a53_defconfig | 1 + configs/evb-ast2600_defconfig| 1 + configs/sama7g5ek_mmc1_defconfig | 1 + configs/sama7g5ek_mmc_defconfig | 1 + lib/Kconfig | 7 +++ lib/fdtdec.c | 7 +-- 6 files changed, 16 insertions(+), 2 deletions(-) >>> >>> Please also see the context here: >>> >>> https://patchwork.ozlabs.org/project/uboot/patch/2020142605.17034-1-a-govindr...@ti.com/ >> >> Previously when we've had issues of making the fast path slow, people >> have posted measurements of before/after in order to demonstrate the >> problem. Can we please get some logs of before/after and various >> possible solutions? Thanks. > > Well this code is not needed at all for all but four boards. Three. One board seems to enable that config for no reason at all. And it wouldn't work on that board, because the code was fragile and error prone. See my detailed explanations in the original patch thread. It does a > very expensive check of the DT and this can happen before relocation, > or is SPL. I don't have a board to test with at present, but I expect > it would cost 10s of milliseconds on AT91, for example. As I've said before, I prefer code which is correct out-of-the-box. I also prefer simpler code with less ifdefs (yeah yeah, IS_ENABLED...) and fewer configuration options to worry about. The three boards which have aliases where the leaf nodes have duplicate names also happen to enable some other unrelated magic config knob which happens to make the phandle comparison work. So at the very least the code should stop comparing phandles and just compare the node offsets; whether that check should be under a config knob can be discussed (certainly that config knob should not have PHANDLE in it), but, again, as I've said, it should be opt-out, and preferably with a build-time check that verifies that no two aliases point at same basenames. _If_ that 10s of milliseconds figure is true, there are other things one could do at build time. Say have some pass over the dtb which simply adds "u-boot,seq" properties to the target nodes of aliases, using that if present, or add a "back pointer" "u-boot,alias" property one could compare to name. Rasmus
Re: [PATCH] Revert "fdtdec: drop needlessly convoluted CONFIG_PHANDLE_CHECK_SEQ"
Hi Tom, On Sun, 3 Jul 2022 at 06:43, Tom Rini wrote: > > On Sun, Jul 03, 2022 at 02:32:42AM -0600, Simon Glass wrote: > > Hi, > > > > On Sun, 3 Jul 2022 at 02:25, Simon Glass wrote: > > > > > > The fdt_path_offset() function is slow since it must scan the tree. > > > This substantial overhead now applies to all boards. > > > > > > The original code may not be ideal but it is fit for purpose and is only > > > needed on a few boards. > > > > > > We should revert this in time for the release. > > > > > > This reverts commit 26f981f295d00351b6f0c69b5317b254b2361cc0. > > > > > > Signed-off-by: Simon Glass > > > --- > > > > > > configs/am65x_evm_a53_defconfig | 1 + > > > configs/evb-ast2600_defconfig| 1 + > > > configs/sama7g5ek_mmc1_defconfig | 1 + > > > configs/sama7g5ek_mmc_defconfig | 1 + > > > lib/Kconfig | 7 +++ > > > lib/fdtdec.c | 7 +-- > > > 6 files changed, 16 insertions(+), 2 deletions(-) > > > > Please also see the context here: > > > > https://patchwork.ozlabs.org/project/uboot/patch/2020142605.17034-1-a-govindr...@ti.com/ > > Previously when we've had issues of making the fast path slow, people > have posted measurements of before/after in order to demonstrate the > problem. Can we please get some logs of before/after and various > possible solutions? Thanks. Well this code is not needed at all for all but four boards. It does a very expensive check of the DT and this can happen before relocation, or is SPL. I don't have a board to test with at present, but I expect it would cost 10s of milliseconds on AT91, for example. Also, it just isn't needed, which is why I suggest a revert. Regards, Simon
Re: [PATCH] Revert "fdtdec: drop needlessly convoluted CONFIG_PHANDLE_CHECK_SEQ"
On Sun, Jul 03, 2022 at 02:32:42AM -0600, Simon Glass wrote: > Hi, > > On Sun, 3 Jul 2022 at 02:25, Simon Glass wrote: > > > > The fdt_path_offset() function is slow since it must scan the tree. > > This substantial overhead now applies to all boards. > > > > The original code may not be ideal but it is fit for purpose and is only > > needed on a few boards. > > > > We should revert this in time for the release. > > > > This reverts commit 26f981f295d00351b6f0c69b5317b254b2361cc0. > > > > Signed-off-by: Simon Glass > > --- > > > > configs/am65x_evm_a53_defconfig | 1 + > > configs/evb-ast2600_defconfig| 1 + > > configs/sama7g5ek_mmc1_defconfig | 1 + > > configs/sama7g5ek_mmc_defconfig | 1 + > > lib/Kconfig | 7 +++ > > lib/fdtdec.c | 7 +-- > > 6 files changed, 16 insertions(+), 2 deletions(-) > > Please also see the context here: > > https://patchwork.ozlabs.org/project/uboot/patch/2020142605.17034-1-a-govindr...@ti.com/ Previously when we've had issues of making the fast path slow, people have posted measurements of before/after in order to demonstrate the problem. Can we please get some logs of before/after and various possible solutions? Thanks. -- Tom signature.asc Description: PGP signature
Re: [PATCH] Revert "fdtdec: drop needlessly convoluted CONFIG_PHANDLE_CHECK_SEQ"
Hi, On Sun, 3 Jul 2022 at 02:25, Simon Glass wrote: > > The fdt_path_offset() function is slow since it must scan the tree. > This substantial overhead now applies to all boards. > > The original code may not be ideal but it is fit for purpose and is only > needed on a few boards. > > We should revert this in time for the release. > > This reverts commit 26f981f295d00351b6f0c69b5317b254b2361cc0. > > Signed-off-by: Simon Glass > --- > > configs/am65x_evm_a53_defconfig | 1 + > configs/evb-ast2600_defconfig| 1 + > configs/sama7g5ek_mmc1_defconfig | 1 + > configs/sama7g5ek_mmc_defconfig | 1 + > lib/Kconfig | 7 +++ > lib/fdtdec.c | 7 +-- > 6 files changed, 16 insertions(+), 2 deletions(-) Please also see the context here: https://patchwork.ozlabs.org/project/uboot/patch/2020142605.17034-1-a-govindr...@ti.com/ Regards, Simon
