Change in coreboot[master]: commonlib/timestamp_serialized.h: Drop outdated remark

List overview All Threads
Download

newer

older

Change in coreboot[master]:...

Angel Pons (Code Review)

22 Feb 2020 22 Feb '20

11:26 p.m.

Angel Pons has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/39084 )

Change subject: commonlib/timestamp_serialized.h: Drop outdated remark ......................................................................

commonlib/timestamp_serialized.h: Drop outdated remark

The timestamps for decompression have been relevant on x86 for a very long time. Henceforth, eliminate the `(ignore for x86)` remarks.

Change-Id: I924e4e2dbb07db49408c77be8f3de59f8c3a8f00 Signed-off-by: Angel Pons th3fanbus@gmail.com --- M src/commonlib/include/commonlib/timestamp_serialized.h 1 file changed, 4 insertions(+), 4 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/84/39084/1

diff --git a/src/commonlib/include/commonlib/timestamp_serialized.h b/src/commonlib/include/commonlib/timestamp_serialized.h index d7d636e..964efd5 100644 --- a/src/commonlib/include/commonlib/timestamp_serialized.h +++ b/src/commonlib/include/commonlib/timestamp_serialized.h @@ -168,10 +168,10 @@ { TS_END_BOOTBLOCK, "end of bootblock" }, { TS_START_COPYROM, "starting to load romstage" }, { TS_END_COPYROM, "finished loading romstage" }, - { TS_START_ULZMA, "starting LZMA decompress (ignore for x86)" }, - { TS_END_ULZMA, "finished LZMA decompress (ignore for x86)" }, - { TS_START_ULZ4F, "starting LZ4 decompress (ignore for x86)" }, - { TS_END_ULZ4F, "finished LZ4 decompress (ignore for x86)" }, + { TS_START_ULZMA, "starting LZMA decompress" }, + { TS_END_ULZMA, "finished LZMA decompress" }, + { TS_START_ULZ4F, "starting LZ4 decompress" }, + { TS_END_ULZ4F, "finished LZ4 decompress" }, { TS_DEVICE_ENUMERATE, "device enumeration" }, { TS_DEVICE_CONFIGURE, "device configuration" }, { TS_DEVICE_ENABLE, "device enable" },

Attachments:

attachment.htm (text/html — 3.5 KB)

Show replies by date

HAOUAS Elyes (Code Review)

22 Feb 22 Feb

11:48 p.m.

HAOUAS Elyes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39084 )

Change subject: commonlib/timestamp_serialized.h: Drop outdated remark ......................................................................

Patch Set 1: Code-Review+2

-- To view, visit https://review.coreboot.org/c/coreboot/+/39084 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I924e4e2dbb07db49408c77be8f3de59f8c3a8f00 Gerrit-Change-Number: 39084 Gerrit-PatchSet: 1 Gerrit-Owner: Angel Pons th3fanbus@gmail.com Gerrit-Reviewer: HAOUAS Elyes ehaouas@noos.fr Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-Comment-Date: Sat, 22 Feb 2020 22:48:18 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

Paul Menzel (Code Review)

23 Feb 23 Feb

12:41 a.m.

Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39084 )

Change subject: commonlib/timestamp_serialized.h: Drop outdated remark ......................................................................

Patch Set 1: Code-Review+1

-- To view, visit https://review.coreboot.org/c/coreboot/+/39084 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I924e4e2dbb07db49408c77be8f3de59f8c3a8f00 Gerrit-Change-Number: 39084 Gerrit-PatchSet: 1 Gerrit-Owner: Angel Pons th3fanbus@gmail.com Gerrit-Reviewer: HAOUAS Elyes ehaouas@noos.fr Gerrit-Reviewer: Paul Menzel paulepanter@users.sourceforge.net Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Julius Werner jwerner@chromium.org Gerrit-Comment-Date: Sat, 22 Feb 2020 23:41:46 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

Patrick Georgi (Code Review)

24 Feb 24 Feb

1:59 p.m.

Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39084 )

Change subject: commonlib/timestamp_serialized.h: Drop outdated remark ......................................................................

Patch Set 1:

Given the comment in the source where the symbol is used (src/lib/cbfs.c:145 /* Note: timestamp not useful for memory-mapped media (x86) */), I guess it's more the issue of not being very useful because it's CPU bound anyway (as compared to platforms where different access strategies can make a big difference).

If so, maybe we should guard the uses there with if (!CONFIG(BOOT_DEVICE_MEMORY_MAPPED)), so that there's no such entry in cbmem on x86 to begin with (and also drop the remark here)?

-- To view, visit https://review.coreboot.org/c/coreboot/+/39084 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I924e4e2dbb07db49408c77be8f3de59f8c3a8f00 Gerrit-Change-Number: 39084 Gerrit-PatchSet: 1 Gerrit-Owner: Angel Pons th3fanbus@gmail.com Gerrit-Reviewer: HAOUAS Elyes ehaouas@noos.fr Gerrit-Reviewer: Paul Menzel paulepanter@users.sourceforge.net Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Julius Werner jwerner@chromium.org Gerrit-CC: Patrick Georgi pgeorgi@google.com Gerrit-Comment-Date: Mon, 24 Feb 2020 12:59:44 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: No Gerrit-MessageType: comment

Angel Pons (Code Review)

4:06 p.m.

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39084 )

Change subject: commonlib/timestamp_serialized.h: Drop outdated remark ......................................................................

Patch Set 1:

...

Patch Set 1:

Given the comment in the source where the symbol is used (src/lib/cbfs.c:145 /* Note: timestamp not useful for memory-mapped media (x86) */), I guess it's more the issue of not being very useful because it's CPU bound anyway (as compared to platforms where different access strategies can make a big difference).

If so, maybe we should guard the uses there with if (!CONFIG(BOOT_DEVICE_MEMORY_MAPPED)), so that there's no such entry in cbmem on x86 to begin with (and also drop the remark here)?

Well, it's nice to have these timestamps. Different (de)compression algorithms need different amounts of time to decompress things.

-- To view, visit https://review.coreboot.org/c/coreboot/+/39084 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I924e4e2dbb07db49408c77be8f3de59f8c3a8f00 Gerrit-Change-Number: 39084 Gerrit-PatchSet: 1 Gerrit-Owner: Angel Pons th3fanbus@gmail.com Gerrit-Reviewer: Angel Pons th3fanbus@gmail.com Gerrit-Reviewer: HAOUAS Elyes ehaouas@noos.fr Gerrit-Reviewer: Paul Menzel paulepanter@users.sourceforge.net Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Julius Werner jwerner@chromium.org Gerrit-CC: Patrick Georgi pgeorgi@google.com Gerrit-Comment-Date: Mon, 24 Feb 2020 15:06:32 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: No Gerrit-MessageType: comment

Julius Werner (Code Review)

25 Feb 25 Feb

1:14 a.m.

Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39084 )

Change subject: commonlib/timestamp_serialized.h: Drop outdated remark ......................................................................

Patch Set 1: Code-Review-1

...

Well, it's nice to have these timestamps. Different (de)compression algorithms need different amounts of time to decompress things.

Sorry, I think you may misunderstand the core problem here: these timestamps cannot accurately reflect decompression speed on memory-mapped SPI devices. On an Arm device, when decompressing an LZMA compressed stage, we first load all the compressed data into CBFS_CACHE (this is the rdev_mmap() in cbfs_load_and_decompress()) and then we run the decompression algorithm. By putting a timestamp in-between those two calls, we can tell how much of the total time was spent on loading and how much on decompression.

But on an x86 device with memory-mapped SPI, rdev_mmap() is pretty much a no-op that just returns a pointer into the memory-mapped region. The data is only actually loaded from SPI at the time the processor accesses that pointer, which it does as part of the decompression function. That's why it's impossible to cleanly separate the time spent on loading and time spent on decompressing, because they're interlocked with each other. That is why this says "ignore on x86" -- because on x86 (at least the memory-mapped ones), the time between TS_START_ULZMA and TS_END_ULZMA includes all the SPI loading time as well (and the start/end of loading stage timestamps don't list any significant extra time on top of that. So there's no value in having the extra decompression timestamps and it's best to ignore them..

I agree with Patrick that just not logging the timestamp at all would probably be a better move, and that it's cleaner to key off CONFIG_BOOT_DEVICE_MEMORY_MAPPED instead of just the architecture. So I think a change in that direction would be useful. But this patch as it is seems wrong -- the reason that remark is there is still very much true today.

(BTW, I think for LZ4 the timings are actually correct because LZ4 does a full rdev_read() before the decompression. But that is inefficient for x86, so the proper fix there would be to change that to use rdev_mmap() on CONFIG_BOOT_DEVICE_MEMORY_MAPPED devices instead.)

-- To view, visit https://review.coreboot.org/c/coreboot/+/39084 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I924e4e2dbb07db49408c77be8f3de59f8c3a8f00 Gerrit-Change-Number: 39084 Gerrit-PatchSet: 1 Gerrit-Owner: Angel Pons th3fanbus@gmail.com Gerrit-Reviewer: Angel Pons th3fanbus@gmail.com Gerrit-Reviewer: HAOUAS Elyes ehaouas@noos.fr Gerrit-Reviewer: Julius Werner jwerner@chromium.org Gerrit-Reviewer: Paul Menzel paulepanter@users.sourceforge.net Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Patrick Georgi pgeorgi@google.com Gerrit-Comment-Date: Tue, 25 Feb 2020 00:14:09 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

Angel Pons (Code Review)

11:13 a.m.

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39084 )

Change subject: commonlib/timestamp_serialized.h: Drop outdated remark ......................................................................

Patch Set 1:

...

Patch Set 1: Code-Review-1

...
Well, it's nice to have these timestamps. Different (de)compression algorithms need different amounts of time to decompress things.

Sorry, I think you may misunderstand the core problem here: these timestamps cannot accurately reflect decompression speed on memory-mapped SPI devices. On an Arm device, when decompressing an LZMA compressed stage, we first load all the compressed data into CBFS_CACHE (this is the rdev_mmap() in cbfs_load_and_decompress()) and then we run the decompression algorithm. By putting a timestamp in-between those two calls, we can tell how much of the total time was spent on loading and how much on decompression.

But on an x86 device with memory-mapped SPI, rdev_mmap() is pretty much a no-op that just returns a pointer into the memory-mapped region. The data is only actually loaded from SPI at the time the processor accesses that pointer, which it does as part of the decompression function. That's why it's impossible to cleanly separate the time spent on loading and time spent on decompressing, because they're interlocked with each other. That is why this says "ignore on x86" -- because on x86 (at least the memory-mapped ones), the time between TS_START_ULZMA and TS_END_ULZMA includes all the SPI loading time as well (and the start/end of loading stage timestamps don't list any significant extra time on top of that. So there's no value in having the extra decompression timestamps and it's best to ignore them..

I agree with Patrick that just not logging the timestamp at all would probably be a better move, and that it's cleaner to key off CONFIG_BOOT_DEVICE_MEMORY_MAPPED instead of just the architecture. So I think a change in that direction would be useful. But this patch as it is seems wrong -- the reason that remark is there is still very much true today.

(BTW, I think for LZ4 the timings are actually correct because LZ4 does a full rdev_read() before the decompression. But that is inefficient for x86, so the proper fix there would be to change that to use rdev_mmap() on CONFIG_BOOT_DEVICE_MEMORY_MAPPED devices instead.)

Oh, I understand what the problem is now. Thanks for the explanation!

-- To view, visit https://review.coreboot.org/c/coreboot/+/39084 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I924e4e2dbb07db49408c77be8f3de59f8c3a8f00 Gerrit-Change-Number: 39084 Gerrit-PatchSet: 1 Gerrit-Owner: Angel Pons th3fanbus@gmail.com Gerrit-Reviewer: Angel Pons th3fanbus@gmail.com Gerrit-Reviewer: HAOUAS Elyes ehaouas@noos.fr Gerrit-Reviewer: Julius Werner jwerner@chromium.org Gerrit-Reviewer: Paul Menzel paulepanter@users.sourceforge.net Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Patrick Georgi pgeorgi@google.com Gerrit-Comment-Date: Tue, 25 Feb 2020 10:13:59 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: No Gerrit-MessageType: comment

Angel Pons (Code Review)

11:14 a.m.

Angel Pons has removed a vote from this change. ( https://review.coreboot.org/c/coreboot/+/39084 )

Change subject: commonlib/timestamp_serialized.h: Drop outdated remark ......................................................................

Removed Code-Review+2 by HAOUAS Elyes ehaouas@noos.fr

-- To view, visit https://review.coreboot.org/c/coreboot/+/39084 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I924e4e2dbb07db49408c77be8f3de59f8c3a8f00 Gerrit-Change-Number: 39084 Gerrit-PatchSet: 1 Gerrit-Owner: Angel Pons th3fanbus@gmail.com Gerrit-Reviewer: Angel Pons th3fanbus@gmail.com Gerrit-Reviewer: HAOUAS Elyes ehaouas@noos.fr Gerrit-Reviewer: Julius Werner jwerner@chromium.org Gerrit-Reviewer: Paul Menzel paulepanter@users.sourceforge.net Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Patrick Georgi pgeorgi@google.com Gerrit-MessageType: deleteVote

Angel Pons (Code Review)

11:14 a.m.

Angel Pons has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/39084 )

Change subject: commonlib/timestamp_serialized.h: Drop outdated remark ......................................................................

Abandoned

Is wrong

-- To view, visit https://review.coreboot.org/c/coreboot/+/39084 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I924e4e2dbb07db49408c77be8f3de59f8c3a8f00 Gerrit-Change-Number: 39084 Gerrit-PatchSet: 1 Gerrit-Owner: Angel Pons th3fanbus@gmail.com Gerrit-Reviewer: Angel Pons th3fanbus@gmail.com Gerrit-Reviewer: HAOUAS Elyes ehaouas@noos.fr Gerrit-Reviewer: Julius Werner jwerner@chromium.org Gerrit-Reviewer: Paul Menzel paulepanter@users.sourceforge.net Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Patrick Georgi pgeorgi@google.com Gerrit-MessageType: abandon

1892

days inactive

1895

days old

coreboot-gerrit@coreboot.org

8 comments

5 participants

tags (0)

participants (5)

Angel Pons (Code Review)
HAOUAS Elyes (Code Review)
Julius Werner (Code Review)
Patrick Georgi (Code Review)
Paul Menzel (Code Review)