From: Mattijs Korpershoek <mkorpershoek@baylibre.com>
To: Tom Rini <trini@konsulko.com>
Cc: qianfan <qianfanguijin@163.com>,
Sean Anderson <sean.anderson@seco.com>,
Marek Vasut <marex@denx.de>,
Guillaume La Roque <glaroque@baylibre.com>,
Gary Bisson <gary.bisson@boundarydevices.com>,
Troy Kisky <troy.kisky@boundarydevices.com>,
Neil Armstrong <neil.armstrong@linaro.org>,
u-boot@lists.denx.de
Subject: Re: [PATCH] lib: sparse: allocate blkcnt instead of arbitrary small number
Date: Fri, 07 Jul 2023 08:52:50 +0200 [thread overview]
Message-ID: <87v8ewdxh9.fsf@baylibre.com> (raw)
In-Reply-To: <20230706170040.GA7930@bill-the-cat>
On jeu., juil. 06, 2023 at 13:00, Tom Rini <trini@konsulko.com> wrote:
> On Thu, Jul 06, 2023 at 11:43:13AM +0200, Mattijs Korpershoek wrote:
>> On lun., juin 19, 2023 at 10:21, Mattijs Korpershoek <mkorpershoek@baylibre.com> wrote:
>>
>> > Hi Qianfan,
>> >
>> > Thank you for your review.
>> >
>> > On lun., juin 19, 2023 at 14:19, qianfan <qianfanguijin@163.com> wrote:
>> >
>> >> 在 2023/6/16 21:26, Mattijs Korpershoek 写道:
>> >>> Commit 62649165cb02 ("lib: sparse: Make CHUNK_TYPE_RAW buffer aligned")
>> >>> fixed cache alignment for systems with a D-CACHE.
>> >>>
>> >>> However it introduced some performance regressions [1] on system
>> >>> flashing huge images, such as Android.
>> >>>
>> >>> On AM62x SK EVM, we also observe such performance penalty:
>> >>> Sending sparse 'super' 1/2 (768793 KB) OKAY [ 23.954s]
>> >>> Writing 'super' OKAY [ 75.926s]
>> >>> Sending sparse 'super' 2/2 (629819 KB) OKAY [ 19.641s]
>> >>> Writing 'super' OKAY [ 62.849s]
>> >>> Finished. Total time: 182.474s
>> >>>
>> >>> The reason for this is that we use an arbitrary small buffer
>> >>> (info->blksz * 100) for transferring.
>> >>>
>> >>> Fix it by using a bigger buffer (info->blksz * blkcnt) as suggested in
>> >>> the original's patch review [2].
>> >>>
>> >>> With this patch, performance impact is mitigated:
>> >>> Sending sparse 'super' 1/2 (768793 KB) OKAY [ 24.006s]
>> >>> Writing 'super' OKAY [ 15.920s]
>> >>> Sending sparse 'super' 2/2 (629819 KB) OKAY [ 19.651s]
>> >>> Writing 'super' OKAY [ 14.665s]
>> >>> Finished. Total time: 74.346s
>> >>>
>> >>> [1] https://lore.kernel.org/r/20221118121323.4009193-1-gary.bisson@boundarydevices.com
>> >>> [2] https://lore.kernel.org/r/all/43e4c17c-4483-ec8e-f843-9b4c5569bd18@seco.com/
>> >>>
>> >>> Fixes: 62649165cb02 ("lib: sparse: Make CHUNK_TYPE_RAW buffer aligned")
>> >>> Signed-off-by: Mattijs Korpershoek <mkorpershoek@baylibre.com>
>> >>> ---
>> >>> lib/image-sparse.c | 2 +-
>> >>> 1 file changed, 1 insertion(+), 1 deletion(-)
>> >>>
>> >>> diff --git a/lib/image-sparse.c b/lib/image-sparse.c
>> >>> index 5ec0f94ab3eb..25aed0604192 100644
>> >>> --- a/lib/image-sparse.c
>> >>> +++ b/lib/image-sparse.c
>> >>> @@ -55,7 +55,7 @@ static lbaint_t write_sparse_chunk_raw(struct sparse_storage *info,
>> >>> void *data,
>> >>> char *response)
>> >>> {
>> >>> - lbaint_t n = blkcnt, write_blks, blks = 0, aligned_buf_blks = 100;
>> >>> + lbaint_t n = blkcnt, write_blks, blks = 0, aligned_buf_blks = blkcnt;
>> >> Hi:
>> >>
>> >> It's a good point that this code report the performance was affected by
>> >> write large small
>> >> mmc blks, not memory copy.
>> >
>> > I believe memory copy also affects performance, but in my case,
>> > it has less impact than small mmc blks.
>> >
>> > With 62649165cb02 reverted:
>> > Sending sparse 'super' 1/2 (768793 KB) OKAY [ 23.947s]
>> > Writing 'super' OKAY [ 12.983s]
>> > Sending sparse 'super' 2/2 (629819 KB) OKAY [ 19.600s]
>> > Writing 'super' OKAY [ 12.796s]
>> > Finished. Total time: 69.430s
>> >
>> > With aligned_buf_blks = blkcnt:
>> > Sending sparse 'super' 1/2 (768793 KB) OKAY [ 24.072s]
>> > Writing 'super' OKAY [ 16.177s]
>> > Sending sparse 'super' 2/2 (629819 KB) OKAY [ 19.681s]
>> > Writing 'super' OKAY [ 14.845s]
>> > Finished. Total time: 74.919s
>> >
>> >>
>> >> And I can not make sure whether memalign can always alloc such huge
>> >> memory when we change the
>> >> aligned_buf_blks to blkcnt.
>> >
>> > Could you clarify the concern here? I've dumped blkcnt for my board
>> > (AM62x SK EVK) and the biggest blkcnt I found was: 131072
>> >
>> > With info->blksz = 512, this gives me: 512 * 131072 = 67108864
>> >
>> > Which is a memalign (memory alloc) of 64MB. Is 64MB really that big? (I
>> > don't realize it's that much)
>> >
>> >>
>> >> Could you please set aligned_buf_blks to FASTBOOT_MAX_BLK_WRITE(16384)
>> >> and test again?
>> >
>> > With aligned_buf_blks = FASTBOOT_MAX_BLK_WRITE(16384):
>> > Sending sparse 'super' 1/2 (768793 KB) OKAY [ 23.912s]
>> > Writing 'super' OKAY [ 15.780s]
>> > Sending sparse 'super' 2/2 (629819 KB) OKAY [ 19.581s]
>> > Writing 'super' OKAY [ 17.192s]
>> > Finished. Total time: 76.569s
>> >
>> > So using FASTBOOT_MAX_BLK_WRITE is slightly worse than using blkcnt.
>> > But allocations (for blksz = 512) are smaller: 8MB instead of 64MB in my example.
>> >
>> > I can spin up a v2 with FASTBOOT_MAX_BLK_WRITE but i'm waiting a little
>> > more feedback before doing so.
>>
>> Hi Marek, Tom,
>>
>> What's your take on this ? Can we keep blkcnt or should I respin using
>> FASTBOOT_MAX_BLK_WRITE ?
>>
>> I have also tested this on VIM3, on
>> U-Boot 2023.07-rc6-00003-g923de765ee1a:
>>
>> Sending sparse 'super' 1/13 (114684 KB) OKAY [ 5.442s]
>> Writing 'super' OKAY [ 5.791s]
>> Sending sparse 'super' 2/13 (114684 KB) OKAY [ 5.706s]
>> Writing 'super' OKAY [ 5.607s]
>> Sending sparse 'super' 3/13 (114684 KB) OKAY [ 5.468s]
>> Writing 'super' OKAY [ 5.835s]
>> Sending sparse 'super' 4/13 (114684 KB) OKAY [ 5.703s]
>> Writing 'super' OKAY [ 5.618s]
>> Sending sparse 'super' 5/13 (114684 KB) OKAY [ 6.176s]
>> Writing 'super' OKAY [ 5.421s]
>> Sending sparse 'super' 6/13 (104176 KB) OKAY [ 5.204s]
>> Writing 'super' OKAY [ 5.199s]
>> Sending sparse 'super' 7/13 (108856 KB) OKAY [ 5.456s]
>> Writing 'super' OKAY [ 5.290s]
>> Sending sparse 'super' 8/13 (114684 KB) OKAY [ 6.122s]
>> Writing 'super' OKAY [ 5.838s]
>> Sending sparse 'super' 9/13 (114684 KB) OKAY [ 5.951s]
>> Writing 'super' OKAY [ 5.857s]
>> Sending sparse 'super' 10/13 (100980 KB) OKAY [ 4.902s]
>> Writing 'super' OKAY [ 4.749s]
>> Sending sparse 'super' 11/13 (114681 KB) OKAY [ 6.041s]
>> Writing 'super' OKAY [ 5.779s]
>> Sending sparse 'super' 12/13 (107212 KB) OKAY [ 5.174s]
>> Writing 'super' OKAY [ 6.587s]
>> Sending sparse 'super' 13/13 (71496 KB) OKAY [ 3.717s]
>> Writing 'super' OKAY [ 3.744s]
>> Finished. Total time: 142.578s
>>
>> With this patch:
>> Sending sparse 'super' 1/13 (114684 KB) OKAY [ 7.149s]
>> Writing 'super' OKAY [ 1.639s]
>> Sending sparse 'super' 2/13 (114684 KB) OKAY [ 6.993s]
>> Writing 'super' OKAY [ 1.713s]
>> Sending sparse 'super' 3/13 (114684 KB) OKAY [ 7.029s]
>> Writing 'super' OKAY [ 1.107s]
>> Sending sparse 'super' 4/13 (114684 KB) OKAY [ 7.027s]
>> Writing 'super' OKAY [ 0.162s]
>> Sending sparse 'super' 5/13 (114684 KB) OKAY [ 6.930s]
>> Writing 'super' OKAY [ 1.643s]
>> Sending sparse 'super' 6/13 (104176 KB) OKAY [ 6.253s]
>> Writing 'super' OKAY [ 2.348s]
>> Sending sparse 'super' 7/13 (108856 KB) OKAY [ 6.346s]
>> Writing 'super' OKAY [ 0.723s]
>> Sending sparse 'super' 8/13 (114684 KB) OKAY [ 6.715s]
>> Writing 'super' OKAY [ 2.848s]
>> Sending sparse 'super' 9/13 (114684 KB) OKAY [ 6.888s]
>> Writing 'super' OKAY [ 1.928s]
>> Sending sparse 'super' 10/13 (100980 KB) OKAY [ 5.979s]
>> Writing 'super' OKAY [ 1.178s]
>> Sending sparse 'super' 11/13 (114681 KB) OKAY [ 6.822s]
>> Writing 'super' OKAY [ 2.652s]
>> Sending sparse 'super' 12/13 (107212 KB) OKAY [ 6.414s]
>> Writing 'super' OKAY [ 5.109s]
>> Sending sparse 'super' 13/13 (71496 KB) OKAY [ 4.238s]
>> Writing 'super' OKAY [ 0.252s]
>> Finished. Total time: 108.151s
>>
>> It's probably too late for v2023.07 to pick this up but can we consider
>> taking it for next?
>
> I was waiting for a v2, and yes, it's too late for v2023.07. Sorry for
> not being clear enough.
Oh, sorry I did not understand that. I understand for v2023.07.
Thank you for the quick answer.
Will send a v2 shortly using FASTBOOT_MAX_BLK_WRITE.
>
> --
> Tom
prev parent reply other threads:[~2023-07-07 6:52 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-06-16 13:26 [PATCH] lib: sparse: allocate blkcnt instead of arbitrary small number Mattijs Korpershoek
2023-06-19 6:19 ` qianfan
2023-06-19 8:21 ` Mattijs Korpershoek
2023-07-06 9:43 ` Mattijs Korpershoek
2023-07-06 17:00 ` Tom Rini
2023-07-07 6:52 ` Mattijs Korpershoek [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=87v8ewdxh9.fsf@baylibre.com \
--to=mkorpershoek@baylibre.com \
--cc=gary.bisson@boundarydevices.com \
--cc=glaroque@baylibre.com \
--cc=marex@denx.de \
--cc=neil.armstrong@linaro.org \
--cc=qianfanguijin@163.com \
--cc=sean.anderson@seco.com \
--cc=trini@konsulko.com \
--cc=troy.kisky@boundarydevices.com \
--cc=u-boot@lists.denx.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.