New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement BufferedBlockDevice #6757

Merged
merged 1 commit into from May 22, 2018

Conversation

Projects
None yet
9 participants
@davidsaada
Contributor

davidsaada commented Apr 26, 2018

Description

Block device allowing smaller read and program sizes for the underlying BD, using a cache.

Pull request type

[ ] Fix
[ ] Refactor
[ ] New target
[X] Feature
[ ] Breaking change
@geky

Looks good to me 👍

@geky geky requested a review from deepikabhavnani Apr 26, 2018

@0xc0170 0xc0170 requested a review from AnotherButler Apr 27, 2018

/** Block device for allowing smaller read and program sizes for the underlying BD,
* using a buffer.

This comment has been minimized.

@0xc0170

0xc0170 Apr 27, 2018

Member

Not sure if this should be extended but first question I have is probably too implementation detailed? the buffer is static, heap? I can see its on the heap.

Why only smaller ? how much smaller ? Is there a range or any limit? What if I have BD that might need bigger reads? What to use?

These might be for documentation or if we can make this more explicit here?

This comment has been minimized.

@davidsaada

davidsaada Apr 27, 2018

Contributor

Changed this line a bit. It is actually minimal and not smaller - read and program sizes are fixed to 1.
Reasoning behind such a block device adaptor is the following: Some BDs have read/program sizes that are too big. For instance, the SD card BD has the value of 512 bytes for both. This can make life very hard for users of these BDs, as they will need to add logics for caching data even when they need to read/write small amounts of it. This adaptor solves the problem. One use case (that was the trigger here) was running StoageLite over SD card.

@davidsaada davidsaada force-pushed the davidsaada:david_buffered_bd branch from 6732ca9 to ee7b040 Apr 27, 2018

int BufferedBlockDevice::flush()
{
if (!_flushed) {
int ret = _bd->program((const void *) _cache, _curr_aligned_addr, _bd_program_size);

This comment has been minimized.

@deepikabhavnani

deepikabhavnani Apr 27, 2018

Contributor

Query: During init we set _curr_aligned_addr = _bd->size();, if flush is called after init, it will program just the last block of the block device or go beyond the size boundary?

Nevermind... Didn't see if (!_flushed)

int BufferedBlockDevice::flush()
{
if (!_flushed) {
int ret = _bd->program((const void *) _cache, _curr_aligned_addr, _bd_program_size);

This comment has been minimized.

@kjbracey-arm

kjbracey-arm Apr 30, 2018

Contributor

I think the cast is unnecessary - hope it is, or something unpleasant is happening.

This comment has been minimized.

@davidsaada

davidsaada Apr 30, 2018

Contributor

Right. Probably was painted red by Eclipse at some point. Fixed.

}
bd_addr_t offs_in_buf = addr - _curr_aligned_addr;
bd_size_t chunk = std::min(_bd_program_size - offs_in_buf, size);
memcpy(buf, (const void *)(_cache + offs_in_buf), chunk);

This comment has been minimized.

@kjbracey-arm

kjbracey-arm Apr 30, 2018

Contributor

Pointless cast

This comment has been minimized.

@davidsaada

davidsaada Apr 30, 2018

Contributor

Ditto.

bd_addr_t aligned_addr = align_down(addr, _bd_program_size);
uint8_t *buf = (uint8_t *) b;

This comment has been minimized.

@kjbracey-arm

kjbracey-arm Apr 30, 2018

Contributor

Should be static_cast<> to check const and assigning to const uint8_t *.

This comment has been minimized.

@davidsaada

davidsaada Apr 30, 2018

Contributor

Correct. Fixed.

// this is covered in the previous condition).
prog_buf = buf;
if (size == _bd_program_size) {
memcpy(_cache, (const void *)buf, _bd_program_size);

This comment has been minimized.

@kjbracey-arm

kjbracey-arm Apr 30, 2018

Contributor

Unnecessary cast

This comment has been minimized.

@davidsaada

davidsaada Apr 30, 2018

Contributor

Ditto.

_bd_program_size = _bd->get_program_size();
if (!_cache) {
_cache = new uint8_t[_bd_program_size];

This comment has been minimized.

@kjbracey-arm

kjbracey-arm Apr 30, 2018

Contributor

If this alloc is being deferred to the error-returning init, does it make sense to use (nothrow) and return an error on memory failure?

This comment has been minimized.

@davidsaada

davidsaada Apr 30, 2018

Contributor

Reason for this being allocated in init is the fact that it relies on the underlying BD's get_program_size, whose value may only be valid after init. Code originally included an assertion for allocation failure, but then was told by @geky that it was redundant, as mbed-os automatically asserts on allocation failures. Is this not correct?

This comment has been minimized.

@kjbracey-arm

kjbracey-arm Apr 30, 2018

Contributor

That's correct - it just seems slightly odd to throw given that all other errors in the function return an error code.

On the other hand, it's not that you really want dynamic allocation with check - if it weren't for that get_program_size you'd be doing a throwing new in the constructor, right? So maybe it's just that, deferred.

I'm generally in favour of throwing for unexpected errors, rather than returning an error no-one checks. So fine.

This comment has been minimized.

@davidsaada

davidsaada Apr 30, 2018

Contributor

I try to distinguish "fatal" errors (like not being able to allocate at init) from "non fatal" ones (errors in read/write for wrong addresses etc.). At least my code (like StorageLite) checks for error returned by the latter. Maybe I should return the assert for allocation failure just for visibility sake.

This comment has been minimized.

@kjbracey-arm

kjbracey-arm Apr 30, 2018

Contributor

I think this is fine - I'll leave it up to you.

This comment has been minimized.

@geky

geky Apr 30, 2018

Member

I think the question is "would an application want to recover from an OOM in the buffered block device?"

int BufferedBlockDevice::deinit()
{
return _bd->deinit();

This comment has been minimized.

@kjbracey-arm

kjbracey-arm Apr 30, 2018

Contributor

Should this free the cache for symmetry? Seems reasonable if they've explicitly called deinit and allocation's in init, but maybe there are practical reasons not to.

This comment has been minimized.

@davidsaada

davidsaada Apr 30, 2018

Contributor

Well, in this specific block device, there's no difference between freeing the cache (or any other resource for that matter) here or in the destructor. However, tried to conform to other block devices, which let the data live through init/deinit calls.

This comment has been minimized.

@geky

geky Apr 30, 2018

Member

Ah! The heap block device is special in this case. (It's "pretending" the heap is non-volatile storage.) I would suggest freeing in deinit also.

This comment has been minimized.

@davidsaada

davidsaada Apr 30, 2018

Contributor

Gotcha. Moved to deinit.

@adbridge adbridge added needs: work and removed needs: review labels Apr 30, 2018

@davidsaada davidsaada force-pushed the davidsaada:david_buffered_bd branch from ee7b040 to df8e360 Apr 30, 2018

@davidsaada

This comment has been minimized.

Contributor

davidsaada commented Apr 30, 2018

Pushed changes following review. Feel free to re-review.

int BufferedBlockDevice::deinit()
{
if (_cache) {

This comment has been minimized.

@kjbracey-arm

kjbracey-arm Apr 30, 2018

Contributor

Don't need the if - always valid to delete or free null pointers.

This comment has been minimized.

@davidsaada

davidsaada May 14, 2018

Contributor

Thanks. Removed it.

@AnotherButler

Nice hyphenation 👍

@AnotherButler

This comment has been minimized.

Contributor

AnotherButler commented Apr 30, 2018

Should we have a user API page for BufferedBlockDevice in the handbook?

@0xc0170

This comment has been minimized.

Member

0xc0170 commented May 3, 2018

Should we have a user API page for BufferedBlockDevice in the handbook?

I believe we should, thus this should wait until PR for the docs is up

@0xc0170

This comment has been minimized.

Member

0xc0170 commented May 3, 2018

/morph build

@mbed-ci

This comment has been minimized.

mbed-ci commented May 3, 2018

Build : SUCCESS

Build number : 1905
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/6757/

Triggering tests

/morph test
/morph uvisor-test
/morph export-build
/morph mbed2-build

@0xc0170 0xc0170 added needs: CI and removed needs: work labels May 3, 2018

@0xc0170

Waiting for docs update (if any, otherwise I'll update the review state)

@davidsaada

This comment has been minimized.

Contributor

davidsaada commented May 3, 2018

Will open a documentation PR when back from GEC.

@mbed-ci

This comment has been minimized.

@mbed-ci

This comment has been minimized.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented May 7, 2018

/morph uvisor-test

@mbed-ci

This comment has been minimized.

mbed-ci commented May 14, 2018

Build : SUCCESS

Build number : 2008
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/6757/

Triggering tests

/morph test
/morph uvisor-test
/morph export-build
/morph mbed2-build

@mbed-ci

This comment has been minimized.

@mbed-ci

This comment has been minimized.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented May 15, 2018

/morph build

@mbed-ci

This comment has been minimized.

mbed-ci commented May 15, 2018

Build : SUCCESS

Build number : 2023
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/6757/

Triggering tests

/morph test
/morph uvisor-test
/morph export-build
/morph mbed2-build

@cmonr

This comment has been minimized.

Contributor

cmonr commented May 15, 2018

/morph build

@cmonr

This comment has been minimized.

Contributor

cmonr commented May 15, 2018

Grrr... I hate that GitHub pages don't auto-refresh...

@mbed-ci

This comment has been minimized.

mbed-ci commented May 15, 2018

Build : SUCCESS

Build number : 2025
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/6757/

Triggering tests

/morph test
/morph uvisor-test
/morph export-build
/morph mbed2-build

static inline uint32_t align_down(bd_size_t val, bd_size_t size)
{
return val / size * size;

This comment has been minimized.

@geky

geky May 15, 2018

Member

In the future, this guy should definitely have parenthesis

*/
virtual bd_size_t size() const;
protected:

This comment has been minimized.

@geky

geky May 15, 2018

Member

Nonblocker, but is there a reason this is protected?

@geky

geky approved these changes May 15, 2018

Looks good to me 👍

@mbed-ci

This comment has been minimized.

@mbed-ci

This comment has been minimized.

@cmonr

This comment has been minimized.

Contributor

cmonr commented May 16, 2018

/morph test

2 similar comments
@cmonr

This comment has been minimized.

Contributor

cmonr commented May 16, 2018

/morph test

@cmonr

This comment has been minimized.

Contributor

cmonr commented May 16, 2018

/morph test

@mbed-ci

This comment has been minimized.

@cmonr

This comment has been minimized.

Contributor

cmonr commented May 18, 2018

/morph test

@mbed-ci

This comment has been minimized.

@0xc0170 0xc0170 added needs: review and removed needs: work labels May 20, 2018

@adbridge

This comment has been minimized.

Contributor

adbridge commented May 21, 2018

@kjbracey-arm @0xc0170 Are you guys happy now with this ?

@cmonr cmonr added ready for merge and removed needs: review labels May 21, 2018

@0xc0170 0xc0170 merged commit fc2f7ff into ARMmbed:master May 22, 2018

13 checks passed

AWS-CI uVisor Build & Test Success
Details
ci-morph-build build completed
Details
ci-morph-exporter build completed
Details
ci-morph-mbed2-build build completed
Details
ci-morph-test test completed
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
travis-ci/astyle Passed, 846 warnings
Details
travis-ci/docs Local docs testing has passed
Details
travis-ci/events Passed, runtime is 8829 cycles (-221 cycles)
Details
travis-ci/gitattributestest Local gitattributestest testing has passed
Details
travis-ci/littlefs Passed, code size is 9964B (+0.00%)
Details
travis-ci/tools Local tools testing has passed
Details

@davidsaada davidsaada deleted the davidsaada:david_buffered_bd branch Jul 9, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment