Add thread safety to CRC class #7781

deepikabhavnani · 2018-08-13T19:57:06Z

Description

Add thread safety to CRC class. Only commit for this PR is 839f46a

Pull request type

[X] Fix
[ ] Refactor
[ ] Target update
[ ] Feature
[ ] Breaking change

Dependent on : #7735

CC @geky

geky · 2018-08-13T21:03:33Z

drivers/MbedCRC.h

        }
-        return 0;
+        unlock();


nit: The series of if statements was cleaner IMO. Not a block, but in the future this may be better written with a series of unlocks in each return statement, or a goto to a cleanup block if the cleanup is too expensive.

Generally gotos are considered harmful, but this is one of the places where a goto makes the intention clearer than a series of nested if statements.

I disagree, had a look again. I would prefer this way as it looks neat, instead of repeated cleanup code / goto. I would have agreed to your point if if statements were 4/5 level deeper.

I tend to agree with @geky, I personally prefer when the expected/successful path of the algorithm is not buried inside nested if statements.

int32_t error = compute_partial_start(crc); if (error) { goto cleanup; } error = compute_partial(buffer, size, crc); if (error) { goto cleanup; } error = compute_partial_stop(crc); if (error) { goto cleanup; } return 0; cleanup: *crc = 0; return error;

geky

Looks good to me, nice test 👍

cmonr · 2018-08-13T21:34:18Z

@deepikabhavnani Please take a look at some of the docs. Looks like Travis is back to normal.

pan-

I'm not sure to understand the design or the use case of the change.

Global/Singleton lock will induce unnecessary contention if multiples different CRCs are computed in parallel.
CRCs are usually not calculated from different sources (do you get some bytes from a thread and other bytes from others threads ?); it is usually not a shared resource either. Why would it be thread safe by default ?

~~I would have split this PR into two prs: one that handle optimized CRC and the other that handles the locking part.~~

pan- · 2018-08-14T10:09:37Z

drivers/MbedCRC.cpp

@@ -22,6 +22,8 @@ namespace mbed {
 /** \addtogroup drivers */
 /** @{*/

+SingletonPtr<PlatformMutex> mbed_crc_mutex;


Why is this a singleton ? It is not possible to have a mutex per MbedCRC instance ?
If the mutex is global then all crc calculations are serialized across threads; I'm not sure this is what we want.

Locking is needed for HW, since we will have one HW module performing CRC computation for all the instances. I understand this is an overhead for software only computations or combination of software and hardware.

We can have lock/unlock only in case of hardware, does that look like correct solution or will cause confusion?

pan- · 2018-08-14T10:11:50Z

drivers/MbedCRC.h

-            *crc = 0;
-            return status;
+        int32_t status = 0;
+        lock();


Could you use RAII lock objects ? It would reduce the code size and ensure no unlock are forgotten.

What makes RAII reduce the code size?

I was mostly referring to the line of code required but in the final binary; we can observe code size reduction too as the compiler is very good at optimizing destruction of objects on the stack.

Here you go: https://godbolt.org/g/nXarPA

It looks like it will behave the same if you have a cleanup label to jump to: https://godbolt.org/z/rgagh-

But good to know gcc can do that automatically for RAII objects. It's a shame it can't figure out that the unlock() calls can be shared.

pan- · 2018-08-14T10:48:21Z

drivers/MbedCRC.h

        }
-        return 0;
+        unlock();


I tend to agree with @geky, I personally prefer when the expected/successful path of the algorithm is not buried inside nested if statements.

int32_t error = compute_partial_start(crc); if (error) { goto cleanup; } error = compute_partial(buffer, size, crc); if (error) { goto cleanup; } error = compute_partial_stop(crc); if (error) { goto cleanup; } return 0; cleanup: *crc = 0; return error;

pan- · 2018-08-14T10:51:54Z

drivers/MbedCRC.h

@@ -264,6 +288,20 @@ class MbedCRC {
        return width;
    }

+    /** Acquire exclusive access to CRC hardware/software
+     */
+    virtual void lock()


Why is this virtual ?
Note: same question for the destructor.

Hmm, I'm guessing to match the other driver classes.

Unrelated, but what are your thoughts on a "Lockable" abstract class to standardize this interface?

What would be the polymorphic use case for such thing ?

We could make the RAII mutex generic for these classes for one.

I was more thinking of this as extra documentation. And that way we make the API is standard.

Use case in my mind was: application if sure of using software only CRC can overwrite these locks to be empty functions.

We could make the RAII mutex generic for these classes for one.

I'm slow today; I still don't see the use case; these class lock and unlock functions aren't supposed to be internal and self contained ?

deepikabhavnani · 2018-08-14T14:47:30Z

Global/Singleton lock will induce unnecessary contention if multiples different CRCs are computed in parallel.

Global/Singleton lock is needed when using HW for CRC computation, we cannot do different algorithms in parallel with HW

CRCs are usually not calculated from different sources (do you get some bytes from a thread and other bytes from others threads ?); it is usually not a shared resource either. Why would it be thread safe by default ?

Agree for software point of view, but it is a share resource in case of HW. Please see https://github.com/ARMmbed/mbed-os/blob/master/targets/TARGET_Freescale/TARGET_MCUXpresso_MCUS/TARGET_MCU_K64F/mbed_crc_api.c and newly added test case https://github.com/ARMmbed/mbed-os/pull/7781/files#diff-add91f4d5e17cbab3ddf004660cf1d4bR137

CRC computation is splitted in 3 parts start compute_partial and stop, if another thread configures hw with different polynomial in between than results won't be correct

pan- · 2018-08-14T15:14:46Z

I do understand that we need to protect the hardware resource that compute CRC from concurrent access with a single mutex as access to that resource must be serialized. However, what I do not understand is why this requirement impacts software computation of the CRC. lock and unlock should do nothing if the mode is not hardware.

CRC computation is splitted in 3 parts start compute_partial and stop, if another thread configures hw with different polynomial in between than results won't be correct

It ain't pretty but it make sense .

deepikabhavnani · 2018-08-14T16:14:25Z

lock and unlock should do nothing if the mode is not hardware.

👍 Will correct the behavior for software in that case and change lock/unlock functions to be non-virtual.

geky · 2018-08-14T16:41:56Z

It's not entirely clear, but the HAL CRC API is designed to keep the current state of the CRC inside hardware registers. I'm guessing this is because fetching the CRC value from the hardware adds a cost that isn't always needed. @scartmell-arm

Here's an example from the user's perspective:

void super_duper_fast_barcode_scanner() {
    Scanner scanner;
    MbedCRC<0xabcdabcd, 32> crc;

    crc.lock(); // lock so no one else messes with the crc hardware while we're using it
    crc.compute_partial_start();

    for (int i = 0; i < 100000000; i++) { // barcode is really long
        uint8_t c = scanner.scan(); // scans one character at a time
        crc.compute_partial(&c, 1);
    }

    uint32_t crc;
    crc.compute_partial_stop(&crc);
    crc.unlock();

    MBED_ASSERT(crc == 0);
}

If this example is correct, the lock/unlock functions are very much a part of the public API. This matches the current API for Serial/SPI and other streaming classes were function calls can end up with interspersed data.

Actually looking at this example raises a few other questions

Why have crc be an argument for start and compute partial? Isn't it only available at the stop stage?
Can start/stop lock/unlock the hardware for the user?

pan- · 2018-08-15T08:47:19Z

I thought again about it and I came to the conclusion that lock/unlock functions should not be exposed to the application side as they are redundant and error prone:

When a CRC is computed by the hardware, the hardware lock must be held for the entirety of the computation. The following sequence would be a programming error if hardware crc is used:

crc.lock(); 
crc.compute_partial_start(...);
crc.unlock()

// get the data

crc.lock(); 
crc.compute_partial(...);
crc.compute_partial_stop(...);
crc.unlock()

The lock should not be released until the computation ends.

Therefore when a CRC is computed with hardware, the function compute_partial_start should acquire the lock and the function compute_partial_stop should release it. If an error happen in compute_partial; the lock can be released too.

deepikabhavnani · 2018-08-15T14:13:29Z

I don't agree with this approach. Scenario above will be a programming error if hardware CRC is used and user should be aware of that. Hence user should have control of lock/unlock.

Also in all the drivers, we give that advantage to users. I would like to be consistent with all the drivers.
https://github.com/ARMmbed/mbed-os/blob/master/drivers/SPI.h#L140
https://github.com/ARMmbed/mbed-os/blob/master/drivers/I2C.h#L148

pan- · 2018-08-15T14:40:03Z

I don't think the comparison with classes such as SPI is right: the SPI class doesn't needs to be locked to be used as locking is internal and the class itself keeps enough data to reconfigure the hardware whenever it acquires the bus. The CRC class doesn't offer that.

In CRC - unlike the SPI API - the flow of a transaction is well defined with prologue and epilogue functions that must be called by the user. Why not use these functions to lock and unlock the mutex associated with the hardware ? They have to be called anyway.

That would make the class thread safe like the other drivers we propose; it isn't at the moment.

deepikabhavnani · 2018-08-15T15:29:36Z

the SPI class doesn't needs to be locked to be used as locking is internal

I believe this is not correct, though you can reconfigure hardware during acquire bus, external locks are still needed. https://github.com/ARMmbed/mbed-os/blob/master/drivers/SPI.h#L65

In case of SD driver as well https://github.com/ARMmbed/sd-driver/blob/master/SDBlockDevice.h#L215,
we have locks as few operations on SPI bus should be done sequentially before device can free the bus.

Consider a application where I am sure only single type of CRC (say CRC-16) for two different modules. In this case start will be called just once during initialization and combination of compute_partial and stop multiple times by both modules and guarded.

If user needs complete serialization then compute should be used. Else I don;t see purpose of other 3 API's.

deepikabhavnani · 2018-08-15T16:55:27Z

Consider a application where I am sure only single type of CRC (say CRC-16) for two different modules. In this case start will be called just once during initialization and combination of compute_partial and stop multiple times by both modules and guarded.

I did testing for this use case and it didn't worked from HAL side. HAL API;s does not pass the previous CRC (which could be used to configure the initial seed).

@geky - Please note compute_partial for LFS will not work for below sequence.

uint32_t crc = 0xffffffff;
compute_partial(&test, sizeof(test), &crc);
crc = 0xffffffff;
compute_partial(&test, sizeof(test), &crc);

It has to be

uint32_t crc = 0xffffffff;
compute(&test, sizeof(test), &crc);
crc = 0xffffffff;
compute(&test, sizeof(test), &crc);

Since we can't do much with compute_partial in hardware without compute_start. It make sense to have 'lock' and 'unlock' as private and set lock in start and only unlock in stop/error. Unless we are planning to update hal to allow setting seed value in compute_partial.

pan-

Ideally the hal would change to accept an opaque state in the function that handles partial computation.

That would allow chunked parallel or interleaved hardware computation of different type of CRCs. As it is right now, the following use case is still broken without changes in the HAL (it has nothing to do with locking ...):

MbedCRC crc_a;
MbedCRC crc_b;

crc_a.compute_partial_start(...);
crc_b.compute_partial_start(...);

crc_a.compute_partial(...);
crc_b.compute_partial(...);

crc_a.compute_partial_stop(...);
crc_b.compute_partial_stop(...);

@scartmell-arm @bulislaw

bulislaw · 2018-08-21T09:53:27Z

Ideally the hal would change to accept an opaque state in the function that handles partial computation.
That would allow chunked parallel or interleaved hardware computation of different type of CRCs. As it is right now, the following use case is still broken without changes in the HAL (it has nothing to do with locking ...):

We considered it, but it wasn't an usecase. The driver should be able to protect the access to HW without bothering user to know when it's necessary to lock the object.

cmonr · 2018-08-21T14:55:25Z

/morph build

cmonr · 2018-08-21T18:35:48Z

@deepikabhavnani Please take a a look at the CI failures.

Thread safety is added to serialize the hardware CRC and will not impact the software CRC.

deepikabhavnani · 2018-08-21T20:27:23Z

@deepikabhavnani Please take a a look at the CI failures.

Forgot to update test application (lock/unlock API's are private in MbedCRC class)

/morph build

geky · 2018-08-21T21:36:16Z

We considered it, but it wasn't an usecase. The driver should be able to protect the access to HW without bothering user to know when it's necessary to lock the object.

How in the world could this work in a multithreaded system? Is only one thread allowed to perform CRC at a time?

cmonr · 2018-08-22T15:52:37Z

@bulislaw Would you mind answering @geky's question?

deepikabhavnani · 2018-08-23T22:10:20Z

@cmonr @geky - With current limitation in hal, yes only one thread allowed to perform CRC at a time.
However, this should be considered in future CRC hal updates.

deepikabhavnani · 2018-08-27T14:47:24Z

@cmonr - Can we start build on this?

0xc0170 · 2018-08-27T14:49:15Z

/morph build

mbed-ci · 2018-08-27T21:14:54Z

Build : SUCCESS

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

Triggering tests

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

mbed-ci · 2018-08-27T23:03:36Z

Exporter Build : SUCCESS

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

mbed-ci · 2018-08-28T13:32:23Z

Test : FAILURE

Build number : 2681
Test logs :http://mbed-os-logs.s3-website-us-west-1.amazonaws.com/?prefix=logs/7781/2681

cmonr · 2018-08-28T15:03:13Z

Will need to retest when CI load is lower.

NirSonnenschein · 2018-08-29T07:46:30Z

/morph test

cmonr · 2018-08-29T14:59:37Z

/morph test

mbed-ci · 2018-08-29T19:08:12Z

Test : SUCCESS

Build number : 2697
Test logs :http://mbed-os-logs.s3-website-us-west-1.amazonaws.com/?prefix=logs/7781/2697

geky reviewed Aug 13, 2018

View reviewed changes

geky approved these changes Aug 13, 2018

View reviewed changes

cmonr added needs: review needs: preceding PR labels Aug 13, 2018

cmonr added needs: work and removed needs: review labels Aug 13, 2018

deepikabhavnani force-pushed the crc_safety branch from 839f46a to 85a0350 Compare August 13, 2018 22:03

pan- reviewed Aug 14, 2018

View reviewed changes

deepikabhavnani force-pushed the crc_safety branch from 85a0350 to c7f6174 Compare August 15, 2018 15:33

deepikabhavnani removed the needs: preceding PR label Aug 15, 2018

deepikabhavnani force-pushed the crc_safety branch from c7f6174 to b00facd Compare August 15, 2018 18:26

pan- approved these changes Aug 15, 2018

View reviewed changes

cmonr added the needs: review label Aug 16, 2018

cmonr requested review from a user and bulislaw August 16, 2018 12:45

cmonr removed the needs: work label Aug 16, 2018

bulislaw approved these changes Aug 21, 2018

View reviewed changes

cmonr added the needs: CI label Aug 21, 2018

Add thread safety to CRC class

986411c

Thread safety is added to serialize the hardware CRC and will not impact the software CRC.

deepikabhavnani force-pushed the crc_safety branch from b00facd to 986411c Compare August 21, 2018 20:26

0xc0170 added needs: review and removed needs: work labels Aug 22, 2018

This was referenced Aug 22, 2018

Template specialization didnt work after addition of default constructor #7793

Merged

Optimized CRC implementation - (32-bit ANSI polynomial) #7735

Merged

cmonr added the release-version: 5.9.7 label Aug 23, 2018

cmonr added release-version: 5.10.0-rc1 and removed release-version: 5.9.7 labels Aug 24, 2018

cmonr added needs: CI and removed needs: review labels Aug 28, 2018

NirSonnenschein added ready for merge and removed needs: CI labels Aug 29, 2018

cmonr merged commit 1e3e694 into ARMmbed:master Aug 29, 2018

0xc0170 removed the ready for merge label Aug 29, 2018

daniel-cesarini-tridonic-com mentioned this pull request Sep 25, 2018

CRC Thread safety #6546

Closed

Add thread safety to CRC class #7781

Add thread safety to CRC class #7781

Uh oh!

Conversation

deepikabhavnani commented Aug 13, 2018

Description

Pull request type

Uh oh!

Choose a reason for hiding this comment

Uh oh!

deepikabhavnani Aug 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

geky left a comment

Choose a reason for hiding this comment

Uh oh!

cmonr commented Aug 13, 2018

Uh oh!

pan- left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

deepikabhavnani Aug 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

deepikabhavnani commented Aug 14, 2018

Uh oh!

pan- commented Aug 14, 2018

Uh oh!

deepikabhavnani commented Aug 14, 2018

Uh oh!

geky commented Aug 14, 2018

Uh oh!

pan- commented Aug 15, 2018

Uh oh!

deepikabhavnani commented Aug 15, 2018

Uh oh!

pan- commented Aug 15, 2018

Uh oh!

deepikabhavnani commented Aug 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

deepikabhavnani commented Aug 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pan- left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bulislaw commented Aug 21, 2018

Uh oh!

cmonr commented Aug 21, 2018

deepikabhavnani Aug 13, 2018 •

edited

Loading

pan- left a comment •

edited

Loading

deepikabhavnani Aug 14, 2018 •

edited

Loading

deepikabhavnani commented Aug 15, 2018 •

edited

Loading

deepikabhavnani commented Aug 15, 2018 •

edited

Loading

pan- left a comment •

edited

Loading

deepikabhavnani commented Aug 21, 2018 •

edited

Loading