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

Event queue #18345

Closed
wants to merge 6 commits into from
Closed

Event queue #18345

wants to merge 6 commits into from

Conversation

paulidale
Copy link
Contributor

Partial implementation of an event queue on top of #18274
This should be enough to get a feel for the design

  • documentation is added or updated
  • tests are added or updated

@paulidale paulidale added the branch: master Merge to master branch label May 19, 2022
@paulidale paulidale self-assigned this May 19, 2022
@paulidale paulidale marked this pull request as draft May 19, 2022 03:16
ssl/priority_queue.c Outdated Show resolved Hide resolved
Copy link
Member

@mattcaswell mattcaswell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Conspicuous by their absence are event queue tests

doc/internal/man3/OSSL_EVENT.pod Outdated Show resolved Hide resolved
doc/internal/man3/OSSL_EVENT.pod Outdated Show resolved Hide resolved
doc/internal/man3/OSSL_EVENT.pod Outdated Show resolved Hide resolved
doc/internal/man3/OSSL_EVENT.pod Outdated Show resolved Hide resolved
doc/internal/man3/OSSL_EVENT.pod Outdated Show resolved Hide resolved
include/internal/event_queue.h Outdated Show resolved Hide resolved
include/internal/event_queue.h Outdated Show resolved Hide resolved
ssl/event_queue.c Outdated Show resolved Hide resolved

if (e != NULL) {
ossl_event_set(e, type, priority, when, ctx, payload, payload_size);
e->flag_dynamic = 1;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes me wonder why we need non-dynamic events.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A non-dynamic event can be embedded into another structure. i.e. cuts overhead allocating and freeing.
E.g. a re-transmit timer event will always exist with a connection -- do we need to allocate it separately?

Dropping non-dynamic is possible, but it feels like it will be useful.

ssl/event_queue.c Outdated Show resolved Hide resolved
ssl/event_queue.c Outdated Show resolved Hide resolved
@paulidale
Copy link
Contributor Author

I didn't do tests because there is no guarantee that this approach will be used.

It definitely isn't quite ready to go at this point.

include/internal/event_queue.h Outdated Show resolved Hide resolved
@github-actions github-actions bot removed the severity: fips change The pull request changes FIPS provider sources label May 24, 2022
@t8m t8m added triaged: feature The issue/pr requests/adds a feature triaged: design The issue/pr deals with a design document labels May 24, 2022
Comment on lines 71 to 76
=item type

A mandatory event type, which is a simple numeric identity, the
meaning of which is not known by the event functionality itself.

=item priority

A manatory nonnegative integral quantity. The lower the priority the earlier
the event will be processes.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I liked the Richard's suggestion from our discussion that a type might have an inherent priority and we would not need a separate priority value.

doc/internal/man3/OSSL_EVENT.pod Outdated Show resolved Hide resolved
doc/internal/man3/OSSL_EVENT.pod Outdated Show resolved Hide resolved
@t8m
Copy link
Member

t8m commented May 24, 2022

A few more comments:

I suppose the events will be sorted by when as the primary key and priority as the secondary key in the priority queue. In that case I wonder how dispatch will work if some high priority event has when in the past but there are multiple lower priority events which have when even further in the past. I suppose these will be dispatched first which is not necessarily correct.

My another concern is with the subscription concept - I wonder whether this isn't overcomplicating things, in a way that requires complicated setup of the subscriptions. Couldn't we have for example a simple static per-type callback table? I mean the callback data and the context should provide enough contextual data. Do we think we will have multiple subscribers for each event type?

@paulidale
Copy link
Contributor Author

paulidale commented May 25, 2022

@t8m, good points.

For dispatch point, it would be possible to extract a list of events that need to run now and sort them by priority rather than time. The main list would be sorted by time only and immediate events would go into the second queue only.

I'm sure about the subscriptions either. There are multiple options (in rough order of complexity):

  • one event handler that dispatches by type -- this is kind of what @levitte did.
  • a global array of static event handlers.
  • a per SSL_METHOD array of static event handlers.
  • installed per type handlers as per this.
  • Event handler associated with the OSSL_EVENT object (caller sets what deals with the event).

They should all be fairly easy to convert between, e.g. the SSL_METHOD could install the per type handlers during initialisation or a single global event hander could dispatch by type to SSL_METHOD handlers.

@paulidale
Copy link
Contributor Author

Embeddable or not?

The reasons for making the event structure embeddable is efficiency. I see two primary use cases:

  • embedded in the caller's event context (the event lives with the event's data)
struct {
    // my fields
    ...
    // event
    OSSL_EVENT event;
} mydata;

ossl_event_queue_add(queue, &mydata->event);
  • embedded with the protocol structure (e.g. re-transmit events)
    Plus a reduction in the number of malloc and free calls.

The reasons against are modularity and good coding practices.

Is this a premature optimisation?

@levitte
Copy link
Member

levitte commented May 25, 2022

  • one event handler that dispatches by type -- this is kind of what @levitte did.

Uhm no. My design allowed multiple subscribers per event type. This was meant to allow a fork where needed...
for example, each stream could be handled by a separate subscriber, where the stream is identified by the indentifiers material attached to the event. With all those subscribers associated with the same queue, I imagine that whatever code that parsed a frame and found the identifying material as well as the payload, would generate one READ event and add it to the queue, and the rest would be a dispatch.

@t8m
Copy link
Member

t8m commented May 25, 2022

Embeddable or not?

I like the concept of embeddable events as I strongly suspect that for reasonable performance the library should avoid mallocs whenever possible. Retrofitting them into the design later when we'll find that the performance is sub-par might be quite a complicated effort.

IMO the biggest concern with embeddable events is the embedded event lifetime - i.e., how to ensure the enclosing object is not prematurely freed, or that the event is forcibly removed from the queue when freeing the enclosing object.

@t8m
Copy link
Member

t8m commented May 25, 2022

  • a per SSL_METHOD array of static event handlers.

I think this should be sufficient for our needs. Unless we want to have some user-supplied callbacks directly used as event handlers for some event types. However I am not sure we need this as the callback(s) can be invoked from the static event handler instead.

IMO refactoring the dispatch if we find out that we really need multiple handlers per event type should not be too big effort.

ssl/time.c Outdated Show resolved Hide resolved
@paulidale paulidale force-pushed the event-queue branch 3 times, most recently from e5c668b to 219d97b Compare June 7, 2022 04:58
Copy link
Member

@hlandau hlandau left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good on the whole.

doc/internal/man3/OSSL_EVENT.pod Outdated Show resolved Hide resolved
doc/internal/man3/OSSL_EVENT.pod Outdated Show resolved Hide resolved
doc/internal/man3/OSSL_EVENT.pod Show resolved Hide resolved
struct ossl_event_st {
uint32_t type; /* What type of event this is */
uint32_t priority; /* What priority this event has */
OSSL_TIME when; /* When the event is scheduled to happen */
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do think we can combine type and priority, and just call it type.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm quite hesitant here. Keeping them distinct adds quite an amount of flexibility.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's tempting to remove type altogether since it's not something the event queue actually cares about anymore.

jquepi pushed a commit to jquepi/openssl.1-.55555 that referenced this pull request Jul 14, 2022
Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from openssl/openssl#18345)
jquepi pushed a commit to jquepi/openssl.1-.55555 that referenced this pull request Jul 14, 2022
Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from openssl/openssl#18345)
jquepi pushed a commit to jquepi/openssl.1-.55555 that referenced this pull request Jul 14, 2022
Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from openssl/openssl#18345)
jquepi pushed a commit to jquepi/openssl.1-.55555 that referenced this pull request Jul 14, 2022
Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from openssl/openssl#18345)
jquepi pushed a commit to jquepi/openssl.1-.55555 that referenced this pull request Jul 14, 2022
Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from openssl/openssl#18345)
jquepi pushed a commit to jquepi/openssl.1-.55555 that referenced this pull request Jul 14, 2022
Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from openssl/openssl#18345)
jquepi pushed a commit to jquepi/openssl.1-.55555 that referenced this pull request Jul 14, 2022
Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from openssl/openssl#18345)
jquepi pushed a commit to jquepi/openssl.1-.55555 that referenced this pull request Jul 14, 2022
Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from openssl/openssl#18345)
jquepi pushed a commit to jquepi/openssl.1-.55555 that referenced this pull request Jul 14, 2022
Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from openssl/openssl#18345)
jquepi pushed a commit to jquepi/openssl.1-.55555 that referenced this pull request Jul 14, 2022
Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from openssl/openssl#18345)
jquepi pushed a commit to jquepi/openssl.1-.55555 that referenced this pull request Jul 14, 2022
Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from openssl/openssl#18345)
jquepi pushed a commit to jquepi/openssl.1-.55555 that referenced this pull request Jul 14, 2022
Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from openssl/openssl#18345)
jquepi pushed a commit to jquepi/openssl.1-.55555 that referenced this pull request Jul 14, 2022
Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from openssl/openssl#18345)
jquepi pushed a commit to jquepi/openssl.1-.55555 that referenced this pull request Jul 14, 2022
Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from openssl/openssl#18345)
jquepi pushed a commit to jquepi/openssl.1-.55555 that referenced this pull request Jul 14, 2022
Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from openssl/openssl#18345)
jquepi pushed a commit to jquepi/openssl.1-.55555 that referenced this pull request Jul 14, 2022
Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from openssl/openssl#18345)
jquepi pushed a commit to jquepi/openssl.1-.55555 that referenced this pull request Jul 14, 2022
Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from openssl/openssl#18345)
jquepi pushed a commit to jquepi/openssl.1-.55555 that referenced this pull request Jul 14, 2022
Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from openssl/openssl#18345)
jquepi pushed a commit to jquepi/openssl.1-.55555 that referenced this pull request Jul 14, 2022
Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from openssl/openssl#18345)
jquepi pushed a commit to jquepi/openssl.1-.55555 that referenced this pull request Jul 14, 2022
Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from openssl/openssl#18345)
jquepi pushed a commit to jquepi/openssl.1-.55555 that referenced this pull request Jul 14, 2022
Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from openssl/openssl#18345)
jquepi pushed a commit to jquepi/openssl.1-.55555 that referenced this pull request Jul 14, 2022
Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from openssl/openssl#18345)
jquepi pushed a commit to jquepi/openssl.1-.55555 that referenced this pull request Jul 14, 2022
Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from openssl/openssl#18345)
jquepi pushed a commit to jquepi/openssl.1-.55555 that referenced this pull request Jul 14, 2022
Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from openssl/openssl#18345)
sftcd pushed a commit to sftcd/openssl that referenced this pull request Sep 24, 2022
Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from openssl#18345)
sftcd pushed a commit to sftcd/openssl that referenced this pull request Sep 24, 2022
Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from openssl#18345)
sftcd pushed a commit to sftcd/openssl that referenced this pull request Sep 24, 2022
Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from openssl#18345)
sftcd pushed a commit to sftcd/openssl that referenced this pull request Sep 24, 2022
Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from openssl#18345)
sftcd pushed a commit to sftcd/openssl that referenced this pull request Sep 24, 2022
Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from openssl#18345)
sftcd pushed a commit to sftcd/openssl that referenced this pull request Sep 24, 2022
Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from openssl#18345)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approval: ready to merge The 24 hour grace period has passed, ready to merge branch: master Merge to master branch triaged: design The issue/pr deals with a design document triaged: feature The issue/pr requests/adds a feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants