Page MenuHomeFreeBSD

loader: tftp: Add preload method
ClosedPublic

Authored by manu on Dec 13 2021, 10:49 AM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Mar 22, 5:19 PM
Unknown Object (File)
Fri, Mar 22, 5:19 PM
Unknown Object (File)
Fri, Mar 8, 12:04 AM
Unknown Object (File)
Jan 12 2024, 7:53 AM
Unknown Object (File)
Jan 11 2024, 4:37 AM
Unknown Object (File)
Dec 20 2023, 5:59 AM
Unknown Object (File)
Dec 7 2023, 6:25 AM
Unknown Object (File)
Nov 3 2023, 12:39 AM

Details

Summary

The preload method will transfer the whole file in a buffer and cache it
so read/lseek operations are faster.

MFC after: 2 weeks
Sponsored by: Beckhoff Automation GmbH & Co. KG

Test Plan
x time-base
+ time-tftp-prefetch
+-----------------------------------------------------------------------------------------------------------------------------+
|+                                                                                                                            |
|+                                                                                                                            |
|+                                                                                                                            |
|+                                                                                                                            |
|+                                                                                                                            |
|+                                                                                                                            |
|+                                                                                                                            |
|+                                                                                                                            |
|+                                                            x                                                               |
|+                                                            xx   x x x     x                    x x                        x|
|A                                                        |___________M________A_____________________|                        |
+-----------------------------------------------------------------------------------------------------------------------------+
    N           Min           Max        Median           Avg        Stddev
x  10           104           193           116         128.9     29.856881
+  10            19            19            19            19             0
Difference at 95.0% confidence
	-109.9 +/- 19.8367
	-85.2599% +/- 2.26839%
	(Student's t, pooled s = 21.112)

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

manu requested review of this revision.Dec 13 2021, 10:49 AM
tsoome added inline comments.
stand/libsa/tftp.c
669

malloc can fail.

672

Probably should be printed if TFTP_DEBUG is defined.

manu marked 2 inline comments as done.
This revision is now accepted and ready to land.Dec 13 2021, 1:37 PM

I was starting to think.... we do have read ahead buffer (f_rabuf) in struct open_file, perhaps we could extend that interface to accommodate preload data?

Core functionality looks good, have some edge case questions.

stand/libsa/tftp.c
673

We're not aborting, we're moving to uncached operations. The calls to preload() don't fail the whole transfer.

686

Do you need to invalidate and flush the cache when there's an error filling it? Maybe it's just as simple as freeing cache.
It might also be nice to have an error message here (pretty sure always, not just under debug, but I'm not 100% sure about that).

stand/libsa/tftp.c
673

Well we are aborting preloading, but if you have better wording I'm ok to change it.

686

Yeah freeing the cache file we avoid leeking.
I'll add that and some error message too.

stand/libsa/tftp.c
673

"Couldn't allocate %ju bytes for preload caching, disabling caching."

This revision now requires review to proceed.Dec 15 2021, 10:06 AM

I like the new messages for failure.
One niggle wrt free, but either way I'm happy now.

stand/libsa/tftp.c
619

You don't need this if. free(NULL) works in the loader.

This revision is now accepted and ready to land.Dec 15 2021, 5:18 PM
stand/libsa/tftp.c
619

Fixed locally.
I'll push the series tomorow.

This revision was automatically updated to reflect the committed changes.

Unfortunately thiss broke tftp kernel loading.
The tftp_open() function will prefetch the first block and leave tftpfile->currblock set to 1, so the first block is never loaded into the cache.
The trivial fix is to reset it back to 0, it works for me but I'm not sure if that's the right solution.

stand/libsa/tftp.c
678
	tftpfile->currblock = 0;
In D33410#760343, @mmel wrote:

Unfortunately thiss broke tftp kernel loading.

I'm obviously using tftp kernel loading and this whole series point was to improve it :)

The tftp_open() function will prefetch the first block and leave tftpfile->currblock set to 1, so the first block is never loaded into the cache.

Mhm, I can see that happening but I wonder why you hit this error while I don't ...

The trivial fix is to reset it back to 0, it works for me but I'm not sure if that's the right solution.

That would work yes but we will restart the transfer, I think that the correct fix should be to copy the data if currclock == 1 and reset to 0 only if currblock > 1 (as we don't have the other block anymore).