Skip to content
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 queue static allocation support #11342

Merged
merged 8 commits into from Aug 30, 2019

Conversation

@maciejbocianski
Copy link
Member

commented Aug 27, 2019

Description

This PR implements mechanism to allow static event posting and dispatching. UserAllocatedEvent provides mechanism for event posting and dispatching without utilization of queue internal memory. UserAllocatedEvent embeds all underlying event data and doesn't require any memory allocation while posting and dispatching. All of these makes it cannot fail due to memory exhaustion while posting.

Docs update will be provided in separate PR

  • adds user allocated event support to equeue
  • update equeue tests
  • adds UserAllocatedEvent implementation
  • update queue test
#include "mbed.h"

void handler(int data) { ... }

class Device {
    public:
    void handler(int data) { ... }
};

Device dev;

// queue with not internal storage for dynamic events
// accepts only user allocated events
static EventQueue queue(0);
// Create events
static auto e1 = make_user_allocated_event(&dev, Device::handler, 2);
static auto e2 = queue.make_user_allocated_event(handler, 3);

int main()
{
   e1.call_on(&queue);
   e2.call();

   queue.dispatch(1);
}

#9172

Pull request type

[ ] Fix
[ ] Refactor
[ ] Target update
[X] Functionality change
[ ] Docs update
[] Test update
[] Breaking change

Reviewers

@kjbracey-arm
@pan-
@jamesbeyond
@geky
@bulislaw

Release Notes

Adds UserAllocatedEvent API to provide mechanism for event posting and dispatching without utilization of queue internal memory.

@ciarmcom ciarmcom requested review from bulislaw, geky, jamesbeyond, kjbracey-arm, pan- and ARMmbed/mbed-os-maintainers Aug 27, 2019
@ciarmcom

This comment has been minimized.

@maciejbocianski maciejbocianski force-pushed the maciejbocianski:event_queue_static_alloc branch 2 times, most recently from 57a59c0 to b61ecec Aug 27, 2019
@kjbracey-arm

This comment has been minimized.

Copy link
Contributor

commented Aug 27, 2019

This looks pretty sweet.

  1. For the examples, could you make those e1 etc be static auto, outside the function, as that's a better demonstration of how they're likely to be used? Do they need a using namespace events?

  2. To match the existing code, you do need the 3 extra c/v/cv overloads for the "object + method" creators like make_user_allocated_event.

  3. It would seem logical for EventQueue to have user_allocated_event methods like its event methods to return ones already bound to it, so you don't have to do the call_on, so

    auto e1 = equeue.user_allocated_event(handler, 2);
    e1();

(Not a huge fan of those being members of EventQueue, rather than external functions taking an EventQueue, but we should stick to existing style).

But on the other hand, such a construction wouldn't be constexpr, so maybe you don't want to encourage it? We kind of want to separate out the constexpr stuff from non-constexpr stuff. (cf the explicit pinmap proposal:

constexpr spi_pinmap_t spi_pins = get_spi_pinmap(D1, D2, D3);
SPI spi(&spi_pins);

)

So maybe leave those out for now.

@maciejbocianski maciejbocianski force-pushed the maciejbocianski:event_queue_static_alloc branch 2 times, most recently from a5036e7 to f6ce710 Aug 27, 2019
@maciejbocianski

This comment has been minimized.

Copy link
Member Author

commented Aug 27, 2019

  1. For the examples, could you make those e1 etc be static auto, outside the function, as that's a better demonstration of how they're likely to be used? Do they need a using namespace events?

using namespace events is placed in "events/mbed_events.h" which is included by "mbed.h"

@@ -21,6 +21,12 @@
#include <stdint.h>
#include <string.h>

// check if the event is allocaded by user - event address is outside queues internal buffer address range
#define IS_USER_ALLOCATED_EVENT(e) (((unsigned int)(e) < (unsigned int)q->buffer) || ((unsigned int)(e) > ((unsigned int)q->slab.data)))

This comment has been minimized.

Copy link
@kjbracey-arm

kjbracey-arm Aug 27, 2019

Contributor

Casting to integer makes it probably safer than comparing pointers (it's undefined behaviour to compare pointers not inside the same object), but the integers should be uintptr_t, or this could go nasty on 64-bit platforms.

This comment has been minimized.

Copy link
@maciejbocianski

maciejbocianski Aug 27, 2019

Author Member

fixed unsigned int -> uintptr_t

// check if the event is allocaded by user - event address is outside queues internal buffer address range
#define IS_USER_ALLOCATED_EVENT(e) (((unsigned int)(e) < (unsigned int)q->buffer) || ((unsigned int)(e) > ((unsigned int)q->slab.data)))
// for user allocated events set id as its address with first bit set
#define MAKE_USER_ALLOCATED_EVENT_ID(e) ((int)(((unsigned int)e) | 1))

This comment has been minimized.

Copy link
@kjbracey-arm

kjbracey-arm Aug 27, 2019

Contributor

Okay, this is potentially a bit icky when we need to squeeze a pointer into the int, We probably get away with it for now, but at some point might need to think about making the int be intptr_t, or actually storing an ID independently inside the object rather than mapping.

This comment has been minimized.

Copy link
@maciejbocianski

maciejbocianski Aug 27, 2019

Author Member

or extend equeue API by void equeue_cancel_by_ptr(equeue_t *queue, struct equeue_event *e); ?

This comment has been minimized.

Copy link
@kjbracey-arm

kjbracey-arm Aug 27, 2019

Contributor

Oh, yes, indeed. No point using ID numbers with user-allocated events.

This comment has been minimized.

Copy link
@maciejbocianski

maciejbocianski Aug 27, 2019

Author Member

But this force changing UserAllocatedEvent API to not provide event id to outside world.
Only UserAllocatedEvent ::cancel will work and EventQueue::cancel will not.
I'm not sure how similar the Event and UserAllocatedEvent APIs should be?

This comment has been minimized.

Copy link
@kjbracey-arm

kjbracey-arm Aug 27, 2019

Contributor

EventQueue::cancel could have an overload accepting a UserAllocatedEvent *.

I don't think there's any fundamental reason for them to mirror each other that degree. For a user-allocated event, an ID isn't fundamentally useful. And the post cannot fail anyway, so making any posts be void-returning enforces that more than just saying "this won't return 0".

This comment has been minimized.

Copy link
@geky

geky Aug 27, 2019

Member

I reached the same conclusion and separated the API into 1. simple call functions, 2. dynamic event functions (void*/ids), 3. static event functions.

Canceling static events with an id also has a performance issue (see review comment).

In the changes I came up with, post returned EBUSY if the static event was coalesced, though I don't know if this is necessary.

return 0;
struct equeue_event *e = 0;
if (IS_USER_ALLOCATED_EVENT_ID(id)) {
equeue_mutex_lock(&q->queuelock);

This comment has been minimized.

Copy link
@kjbracey-arm

kjbracey-arm Aug 27, 2019

Contributor

Could pull this lock outside the if, just to minimise code

This comment has been minimized.

Copy link
@geky

geky Aug 27, 2019

Member

You want the lock to contain the minimum amount of code though. It's a critical section, so every cycle counts.

This comment has been minimized.

Copy link
@kjbracey-arm

kjbracey-arm Aug 28, 2019

Contributor

I certainly agree with the general principle, but not convinced putting a single if (id & 1) inside a critical section is enough extra overhead to be worth increasing code size to avoid.

The critical section would be better served by eliminating the list search by removing the use of IDs for static events, as suggested previously.

@maciejbocianski maciejbocianski force-pushed the maciejbocianski:event_queue_static_alloc branch from f6ce710 to 6703a46 Aug 27, 2019
@geky

This comment has been minimized.

Copy link
Member

commented Aug 27, 2019

Ah! First glance this looks like a good solution to static events if you must maintain backwards compatibility. Downside is you're limiting yourself hardware-wise. Likely a great solution for Mbed OS though.

Will review.

Just an FYI, I've been working on something similar, but with reversed requirements: Portable but with backwards compatibility not a concern. It's unfortunate GitHub really doesn't have a way to collaborate before PR:
https://github.com/geky/equeue/compare/v2-alpha

(FYI @kjbracey-arm that branch also has a priority mechanism, event coalescing, and standardization on POSIX-esque error codes)

#define IS_USER_ALLOCATED_EVENT(e) (((uintptr_t)(e) < (uintptr_t)q->buffer) || ((uintptr_t)(e) > ((uintptr_t)q->slab.data)))
// for user allocated events set id as its address with first bit set
#define MAKE_USER_ALLOCATED_EVENT_ID(e) ((int)(((uintptr_t)e) | 1))
#define IS_USER_ALLOCATED_EVENT_ID(id) (((id) & 1) == 1)

This comment has been minimized.

Copy link
@geky

geky Aug 27, 2019

Member

nit: Would suggest prefixing these with EQUEUE_ for consistency.

This comment has been minimized.

Copy link
@maciejbocianski

maciejbocianski Aug 27, 2019

Author Member

fixed

return 0;
struct equeue_event *e = 0;
if (IS_USER_ALLOCATED_EVENT_ID(id)) {
equeue_mutex_lock(&q->queuelock);

This comment has been minimized.

Copy link
@geky

geky Aug 27, 2019

Member

You want the lock to contain the minimum amount of code though. It's a critical section, so every cycle counts.

if (IS_USER_ALLOCATED_EVENT_ID(id)) {
equeue_mutex_lock(&q->queuelock);
struct equeue_event *cur = q->queue;
while (cur) {

This comment has been minimized.

Copy link
@geky

geky Aug 27, 2019

Member

Does this work if static events are set to go off at the same tick? equeue uses a 2d linked-list so the events would become siblings. I believe you need to follow the sibling pointers as well.

Something to be concerned about: this is a O(n) loop in a critical section. Currently equeue promises a O(1*ticks) in critical sections. Not a deal breaker, just worth noting.

My conclusion was that a pointer-based cancel function and erroring on traditional cancel with a static event was the correct solution for performance and usability reasons. Any user with a static event will have its address ready to go anyways, so that's one less pointer for them to store.

This comment has been minimized.

Copy link
@maciejbocianski

maciejbocianski Aug 28, 2019

Author Member

good catch
I fixed it and updated tests to cover this

}


// equeue scheduling functions
static int equeue_enqueue(equeue_t *q, struct equeue_event *e, unsigned tick)

This comment has been minimized.

Copy link
@geky

geky Aug 27, 2019

Member

It's worth asking: What happens if you post the same static event twice? I believe the current code explodes as equeue tries to append the event as its own sibling? Not really sure what happens.

@flit made the good point that static events should coalesce in this case. This makes static events a nice alternative to binary semaphores/eventflags if they aren't available on your system. (and maybe helps with all the corner cases around tear down of event heavy classes @kjbracey-arm?).

This logic takes care of coalescing:
https://github.com/geky/equeue/blob/e50bf24cc82874b694e643e521498ed357647680/equeue.c#L216-L221

(The coalesce flag is because that implementation has two separate post entry points for dynamic and static events, it could be modified to use your IS_USER_ALLOCATED_EVENT macro).

This comment has been minimized.

Copy link
@maciejbocianski

maciejbocianski Aug 28, 2019

Author Member

It's a problem but we assumed that such situation will not happen, but API allow this.
@kjbracey-arm maybe to add more safety to UserAllocatedEvent we should leave only bool try_call API to prevent double posting ? It will also ease canceling, only UserAllocatedEvent::cancel will work

This comment has been minimized.

Copy link
@kjbracey-arm

kjbracey-arm Aug 28, 2019

Contributor

I feel strongly that it should be possible to safely make a second queue call and have it ignored via some API, so there's no need for separate "have I queued" tracking logic in the client - a sigio handler can just do post().

Maybe this usage should be distinguished via try_post (return bool/error) versus post (return void, assert on fail).

The other plausible behaviour (which might be preferable when we can pass parameters) would be to actually have a second queue attempt replace the first one (updating parameters, but retaining position in queue). That's indistinguishable if you're passing no parameters.

@maciejbocianski maciejbocianski force-pushed the maciejbocianski:event_queue_static_alloc branch 2 times, most recently from 83f35be to f74a36b Aug 27, 2019
@maciejbocianski

This comment has been minimized.

Copy link
Member Author

commented Aug 28, 2019

  1. To match the existing code, you do need the 3 extra c/v/cv overloads for the "object + method" creators like make_user_allocated_event.

fixed

Copy link
Member

left a comment

I don't have time to review completely the PR today however I skimmed through it and I'm concerned by the lack of extensibility if an application uses its own allocators and allocate events with them. Wouldn't it be possible to use equeue_event::dtor as a function pointer that release all resources associated with the event; not just destroying the event. Just like a smart pointer destructor it would destroy the pointed object and release it. It would also prevent pointer tagging.

@maciejbocianski @kjbracey-arm @geky What do you think ?

@maciejbocianski

This comment has been minimized.

Copy link
Member Author

commented Aug 28, 2019

  1. It would seem logical for EventQueue to have user_allocated_event methods like its event methods to return ones already bound to it, so you don't have to do the call_on, so

@kjbracey-arm
Maybe better idea is to add make_user_allocated_event_bound rather than EventQueue extending?

Copy link
Member

left a comment

I'd really like to have a mode that wouldn't pull any dynamic memory in at any stage. Also could you update the doxygen of equeue and even queue to mention the "static" mode and how to use it.

* {
* // queue with not internal storage for dynamic events
* // accepts only user allocated events
* EventQueue queue(1);

This comment has been minimized.

Copy link
@bulislaw

bulislaw Aug 28, 2019

Member

That still requires malloc (even if of 1). Could we have a special mode that doesn't pull the malloc in at all?

This comment has been minimized.

Copy link
@kjbracey-arm

kjbracey-arm Aug 28, 2019

Contributor

Yes, you should be able to just do queue(0) if intending only to use static events.

(With current code, you could do queue(1, &my_single_char) to avoid the malloc.)

This comment has been minimized.

Copy link
@maciejbocianski

maciejbocianski Aug 28, 2019

Author Member

or do it this way

 EventQueue::EventQueue(unsigned event_size, unsigned char *event_pointer)
 {
+    if (event_size == 0) {
+        static uint8_t dummy;
+        equeue_create_inplace(&_equeue, event_size, &dummy);
+    } else {
     if (!event_pointer) {
         equeue_create(&_equeue, event_size);
     } else {

This comment has been minimized.

Copy link
@kjbracey-arm

kjbracey-arm Aug 29, 2019

Contributor

Looks good, assuming it never touches dummy. And if it REALLY doesn't ever touch dummy, you could, in principle just pass &_equeue as your buffer instead, saving 1 byte of RAM (in all images - that static dummy gets reserved whatever).

This comment has been minimized.

Copy link
@maciejbocianski

maciejbocianski Aug 29, 2019

Author Member

I encountered that doing it via static uint8_t dummy; cause some strange run time errors.
e.g. this assert

MBED_ASSERT(p == reinterpret_cast<T *>(&_data));

So for now dummy is regular EventQueue member.
I think that 1B more per queue is not much.
How many queues utilize typical program?

This comment has been minimized.

Copy link
@kjbracey-arm

kjbracey-arm Aug 29, 2019

Contributor

That sounds dangerous. As that assert notes, if it's triggered, someone has corrupted memory. Which suggests that your attempt to do that has made it run off the end of _dummy. It may still be doing that, with the dummy elsewhere, and you're not noticing.

Is there some assumption that the buffer is at least some size, and it's writing in a fixed header?

You could catch that with a watchpoint on _dummy + 1.

This comment has been minimized.

Copy link
@maciejbocianski

maciejbocianski Aug 29, 2019

Author Member

fixed. The problem was in queue buffer alignment

int equeue_create_inplace(equeue_t *q, size_t size, void *buffer)
{
    // setup queue around provided buffer
    // ensure buffer and size are aligned
    if (size >= sizeof(void *)) {
        q->buffer = (void *)(((uintptr_t) buffer + sizeof(void *) -1) & ~(sizeof(void *) -1));
        size -= (char *) q->buffer - (char *) buffer;
        size &= ~(sizeof(void *) -1);
    } else {
        // don't align when size less then pointer size
        // e.g. static queue (size == 1)
        q->buffer = buffer;
    }

This comment has been minimized.

Copy link
@kjbracey-arm

kjbracey-arm Aug 29, 2019

Contributor

Cool, but why is it accessing the buffer at all, even after alignment?

@kjbracey-arm

This comment has been minimized.

Copy link
Contributor

commented Aug 28, 2019

  1. It would seem logical for EventQueue to have user_allocated_event methods like its event methods to return ones already bound to it, so you don't have to do the call_on, so

@kjbracey-arm
Maybe better idea is to add make_user_allocated_event_bound rather than EventQueue extending?

If you were to do that, then for symmetry you'd kind of want make_event_bound too. I don't feel that strongly either way on that.

I'm more concerned about maintaining constant initialisation for EventQueues - I was uncertain whether this would be legal constant initialisation, but I now think it is:

static EventQueue my_event_queue;
static auto my_event = make_user_allocated_event_bound(&my_event_queue, blah);

&my_event_queue is an address constant expression, so if make_user_allocated_event is constexpr, the whole thing is a constant expression, and my_event gets constant initialised. (Which means it is efficient code and the object can be linker dropped if unused - no need for SingletonPtr). It does mean the event gets constructed before the EventQueue, but shouldn't matter as long as you don't post the event before main.

But it's still easy to fail in practice - it wouldn't work with get_shared_event_queue, or if you wrapped my_event_queue in a SingletonPtr - the resulting EventQueue * would not be a constant. (I'm not sure whether those could be re-engineered to make it work. Doubtful).

The upshot of all that is that it will generally be easier to construct your events unbound, and bind them later. There should at least be an API to do just a bind during an init, so all posts can just operator(). Three stages - constant init, binding, posting.

Copy link
Contributor

left a comment

Okay, looks fine to me, but change the in-place to 0, nullptr if you can. (If it passes CI like this, you can leave it).

@maciejbocianski

This comment has been minimized.

Copy link
Member Author

commented Aug 30, 2019

EventQueue::EventQueue(unsigned event_size, unsigned char *event_pointer)
{
    if (event_size == 0) {
        equeue_create_inplace(&_equeue, 0, NULL);
    } else {

Setting 0 size looks OK
But buffer address is used here

// check if the event is allocaded by user - event address is outside queues internal buffer address range
#define EQUEUE_IS_USER_ALLOCATED_EVENT(e) (((uintptr_t)(e) < (uintptr_t)q->buffer) || ((uintptr_t)(e) > ((uintptr_t)q->slab.data)))

But this also should be OK, will test it

@kjbracey-arm

This comment has been minimized.

Copy link
Contributor

commented Aug 30, 2019

Hmm, skirting deeper into undefined behaviour territory I guess. But as it's not null pointer CONSTANT, only a null pointer, I can't see it causing extra issues. (Except LTO?)

You could add a leading q->buffer == NULL || to that test, which would shut that issue down.

@mbed-ci

This comment has been minimized.

Copy link

commented Aug 30, 2019

Test run: FAILED

Summary: 1 of 4 test jobs failed
Build number : 2
Build artifacts

Failed test jobs:

  • jenkins-ci/mbed-os-ci_build-ARM
@0xc0170

This comment has been minimized.

Copy link
Member

commented Aug 30, 2019

CI restarted

@maciejbocianski

This comment has been minimized.

Copy link
Member Author

commented Aug 30, 2019

Fails looks not related

@maciejbocianski

This comment has been minimized.

Copy link
Member Author

commented Aug 30, 2019

@kjbracey-arm
changed to

EventQueue::EventQueue(unsigned event_size, unsigned char *event_pointer)
{
    if (event_size == 0) {
        // As static queue (EventQueue(0)) won't perform any access to its data buffer
        // we can pass (0, NULL)
        equeue_create_inplace(&_equeue, 0, NULL);
    } else {

e7a953d

equeue_create_inplace(&_equeue, 1, this);
// As static queue (EventQueue(0)) won't perform any access to its data buffer
// we can pass (0, NULL)
equeue_create_inplace(&_equeue, 0, NULL);

This comment has been minimized.

Copy link
@kjbracey-arm

kjbracey-arm Aug 30, 2019

Contributor

In general should be shifting to nullptr in C++ code, but doesn't matter when calling C code anyway.

UserAllocatedEvent provides mechanism for event posting and dispatching without
utilization of queue internal memory. UserAllocatedEvent embeds all underlying
event data and doesn't require any memory allocation while posting and dispatching.
All of these makes it cannot fail due to memory exhaustion while posting.
without this fix test_equeue_break_no_windup was failing on IAR
@maciejbocianski maciejbocianski force-pushed the maciejbocianski:event_queue_static_alloc branch from e7a953d to 4c3889d Aug 30, 2019
@mbed-ci

This comment has been minimized.

Copy link

commented Aug 30, 2019

Test run: FAILED

Summary: 3 of 4 test jobs failed
Build number : 3
Build artifacts

Failed test jobs:

  • jenkins-ci/mbed-os-ci_build-ARM
  • jenkins-ci/mbed-os-ci_build-IAR
  • jenkins-ci/mbed-os-ci_build-GCC_ARM
@0xc0170

This comment has been minimized.

Copy link
Member

commented Aug 30, 2019

Aborted, restarting

@0xc0170 0xc0170 added needs: CI and removed needs: review labels Aug 30, 2019
@0xc0170

This comment has been minimized.

Copy link
Member

commented Aug 30, 2019

Started CI after astyle fix

@mbed-ci

This comment has been minimized.

Copy link

commented Aug 30, 2019

Test run: SUCCESS

Summary: 11 of 11 test jobs passed
Build number : 4
Build artifacts

@0xc0170

This comment has been minimized.

Copy link
Member

commented Aug 30, 2019

Another CI needs to finish after astyle fix

@mbed-ci

This comment has been minimized.

Copy link

commented Aug 30, 2019

Test run: SUCCESS

Summary: 11 of 11 test jobs passed
Build number : 5
Build artifacts

@0xc0170 0xc0170 added ready for merge and removed needs: CI labels Aug 30, 2019
@0xc0170 0xc0170 merged commit bdd6cb8 into ARMmbed:master Aug 30, 2019
25 checks passed
25 checks passed
continuous-integration/jenkins/pr-head This commit looks good
Details
jenkins-ci/build-ARM Success
Details
jenkins-ci/build-GCC_ARM Success
Details
jenkins-ci/build-IAR Success
Details
jenkins-ci/cloud-client-test Success
Details
jenkins-ci/dynamic-memory-usage RTOS ROM(+0 bytes) RAM(+0 bytes)
Details
jenkins-ci/exporter Success
Details
jenkins-ci/greentea-test Success
Details
jenkins-ci/mbed2-build-ARM Success
Details
jenkins-ci/mbed2-build-GCC_ARM Success
Details
jenkins-ci/mbed2-build-IAR Success
Details
jenkins-ci/unittests Success
Details
travis-ci/astyle Success!
Details
travis-ci/docs Success!
Details
travis-ci/doxy-spellcheck Success!
Details
travis-ci/events Success! Runtime is 8979 cycles.
Details
travis-ci/gitattributestest Success!
Details
travis-ci/include_check Success!
Details
travis-ci/licence_check Success!
Details
travis-ci/littlefs Success! Code size is 8464B.
Details
travis-ci/psa-autogen Success!
Details
travis-ci/tools-py2.7 Success!
Details
travis-ci/tools-py3.5 Success!
Details
travis-ci/tools-py3.6 Success!
Details
travis-ci/tools-py3.7 Success!
Details
dmaziec1 pushed a commit to dmaziec1/mbed-os that referenced this pull request Sep 5, 2019
…tic_alloc

add queue static allocation support
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
9 participants
You can’t perform that action at this time.