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

Add init reference count to all block devices #7648

Merged
merged 1 commit into from Aug 1, 2018

Conversation

Projects
None yet
5 participants
@davidsaada
Contributor

davidsaada commented Jul 30, 2018

Description

This PR fixes the problem of multiple calls to BD init/deinit APIs, by simply adding an init ref count. This is required due to the fact that utility block devices, which include underlying block devices, call the underlying init/deinit APIs automatically.
This PR partially resolves #7455 (for all BDs included in mbed-os). External BDs should be handled in a different PR.

Pull request type

[x] Fix
[ ] Refactor
[ ] New target
[ ] Feature
[ ] Breaking change

@davidsaada davidsaada force-pushed the davidsaada:david_init_ref_count branch from 0821855 to 71fd582 Jul 30, 2018

@cmonr cmonr requested a review from ARMmbed/mbed-os-storage Jul 30, 2018

@cmonr cmonr added the needs: review label Jul 30, 2018

if (_init_ref_count++) {
return BD_ERROR_OK;
}

This comment has been minimized.

@geky

geky Jul 30, 2018

Member

@davidsaada, I think I'm missing something. Is this needed if we can expect the underlying block device to also be reference counted?

This comment has been minimized.

@davidsaada

davidsaada Jul 31, 2018

Contributor

Yes it is. If the block device has a resource (like BufferedBlockDevice) it would make sense to protect it against the scenario described in the issue.
Now, while it is not needed in devices like ObservingBlockDevice, ones not having any allocated resources, it is harmless, and it will become effective if one decides to add such a resource in the future. This is why I think it's a good practice to have this in all block devices, also for reference sake for future ones (which may need it).

@davidsaada

This comment has been minimized.

Contributor

davidsaada commented Jul 31, 2018

@@ -36,6 +36,10 @@ BufferedBlockDevice::~BufferedBlockDevice()
int BufferedBlockDevice::init()
{
if (_init_ref_count++) {

This comment has been minimized.

@0xc0170

0xc0170 Jul 31, 2018

Member

ref_count operations do not need to be atomic?

This comment has been minimized.

@davidsaada

davidsaada Jul 31, 2018

Contributor

Absolutely right. Fixed now.

@davidsaada davidsaada force-pushed the davidsaada:david_init_ref_count branch 2 times, most recently from 044a52b to 75afa44 Jul 31, 2018

@geky

This comment has been minimized.

Member

geky commented Jul 31, 2018

Is this needed if we can expect the underlying block device to also be reference counted?

Now, while it is not needed in devices like ObservingBlockDevice, ones not having any allocated resources, it is harmless, and it will become effective if one decides to add such a resource in the future. This is why I think it's a good practice to have this in all block devices, also for reference sake for future ones (which may need it).

That's fair, but would a comment convey the same info without the extra code cost?

@geky

Sorry I didn't see the above issue earlier, though I believe it's an easy fix.

@@ -217,6 +218,10 @@ class BlockDevice
size % get_erase_size() == 0 &&
addr + size <= this->size());
}
protected:
uint32_t _init_ref_count;

This comment has been minimized.

@geky

geky Jul 31, 2018

Member

Sorry I didn't see this earlier. We should not have state in interfaces classes.

This adds a RAM cost to every block device class. Even ones with their own form of synchronization (ie SD, SPIF, others outside of tree).

At worst this can discourage users from applying the interface to their own code. Interfaces work best when a class satisfies as many interfaces as possible, allowing code reuse for functions that use that interface. If there's extra state in an interface, users may not adopt the interface because of the code cost. Even if it makes sense otherwise.

This is what happened with the FileLike/FileSystemLike classes. State was introduced for constructing the file tree, which on paper was a good idea, but now the interface is avoided and the poorer-named-but-stateless FileHandle class is used instead. (We still have to put up with this because of backwards compatibility, but it was a lesson learned.)


I believe this can be moved into each of the implementing classes without any cost?

This comment has been minimized.

@davidsaada

davidsaada Jul 31, 2018

Contributor

Will move. Certainly now that not all classes implement it.

This comment has been minimized.

@geky

geky Jul 31, 2018

Member

Thanks. The FileLike/FileSystemLike classes have been frustrating, and I didn't want to see us repeat it.

@davidsaada

This comment has been minimized.

Contributor

davidsaada commented Jul 31, 2018

That's fair, but would a comment convey the same info without the extra code cost?

Fair enough. Will replace it with a comment in the resource-less classes (in the the spirit of the current efforts to reduce code size). Would suggest these include Observing, Profiling, ReadOnly & slicing.

@0xc0170 0xc0170 added needs: work and removed needs: review labels Jul 31, 2018

@davidsaada davidsaada force-pushed the davidsaada:david_init_ref_count branch from 75afa44 to 235f175 Jul 31, 2018

@davidsaada

This comment has been minimized.

Contributor

davidsaada commented Jul 31, 2018

Pushed fixes conforming to comments given by @geky.

@cmonr

This comment has been minimized.

Contributor

cmonr commented Jul 31, 2018

@geky Mind re-reviewing?

@cmonr cmonr added needs: review and removed needs: work labels Jul 31, 2018

@geky

geky approved these changes Jul 31, 2018 edited

Looks great to me 👍

Thanks for taking care of my complaints

@cmonr

This comment has been minimized.

Contributor

cmonr commented Jul 31, 2018

/morph build

@cmonr

cmonr approved these changes Jul 31, 2018

@cmonr

This comment has been minimized.

Contributor

cmonr commented Jul 31, 2018

@ARMmbed/mbed-os-maintainers Targeting this for a patch even though a parameter has been added to block device constructors. Guidance: https://github.com/ARMmbed/mbed_os_release_notes/pull/7/files#diff-895497eee5018cac7ab732faa7d51128R33

@mbed-ci

This comment has been minimized.

mbed-ci commented Jul 31, 2018

Build : SUCCESS

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

Triggering tests

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

@mbed-ci

This comment has been minimized.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Aug 1, 2018

@ARMmbed/mbed-os-maintainers Targeting this for a patch even though a parameter has been added to block device constructors.

just a private member that gets initial value (can't be set via public API anyhow). This breaks ABI (fine for any release - recompilation needed)

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Aug 1, 2018

/morph export-build

@mbed-ci

This comment has been minimized.

@mbed-ci

This comment has been minimized.

@0xc0170 0xc0170 added ready for merge and removed needs: CI labels Aug 1, 2018

@0xc0170 0xc0170 merged commit d65e614 into ARMmbed:master Aug 1, 2018

14 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, 793 files
Details
travis-ci/docs Local docs testing has passed
Details
travis-ci/events Passed, runtime is 10381 cycles
Details
travis-ci/gitattributestest Local gitattributestest testing has passed
Details
travis-ci/licence_check Local licence_check testing has passed
Details
travis-ci/littlefs Passed, code size is 9960B
Details
travis-ci/tools-py2.7 Local tools-py2.7 testing has passed
Details

@0xc0170 0xc0170 removed the ready for merge label Aug 1, 2018

@davidsaada davidsaada deleted the davidsaada:david_init_ref_count branch Aug 1, 2018

pan- pushed a commit to pan-/mbed that referenced this pull request Aug 22, 2018

Merge pull request ARMmbed#7648 from davidsaada/david_init_ref_count
Add init reference count to all block devices
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment