Skip to content

Patch/make skip reason clearer #3

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

PerMac
Copy link
Owner

@PerMac PerMac commented Aug 25, 2022

No description provided.

@PerMac PerMac force-pushed the patch/make_skip_reason_clearer branch 6 times, most recently from c5c5fe4 to b97d315 Compare August 30, 2022 14:41
@nashif nashif force-pushed the patch/make_skip_reason_clearer branch from b97d315 to a539a46 Compare September 15, 2022 18:10
@PerMac PerMac force-pushed the patch/make_skip_reason_clearer branch from a539a46 to 3596aef Compare September 15, 2022 18:25
@PerMac PerMac force-pushed the patch/make_skip_reason_clearer branch 8 times, most recently from a9d8b42 to 7d1f9af Compare October 13, 2022 11:32
@PerMac PerMac force-pushed the patch/make_skip_reason_clearer branch 2 times, most recently from 7f89019 to e763ef1 Compare October 21, 2022 12:51
There is no point in executing check_runnable() twice: when loading
test data from yaml and when filters are applied during test plan
creation. The check is left only in the latter case.
"tfilter" variable is removed since the same logic can be preserved
without it by directly using the "runnable" one.
Checks for harness and fixtures are encapsulated as individual methods.
Now checking fixtures both from cli and hwmap happens in a single
place. The check for runnability is tangled with apply_filter() method.
This commit makes the apply_filter() to be executed for all twisters
workflows to improve consitency of the results. Before, e.g. the
filters were not applied when --test-only was used. This would cause
errors if tests are first build with --build-only on one setup and
then ported to another setup with different configuration, where some
requirements (e.g. fixture) are not met.
The changes are followed with an update to twister unit tests to
reflect the modifications.

Signed-off-by: Maciej Perkowski <[email protected]>
The skip reason was improved by clearly stating
which keyword is not supported.

Signed-off-by: Maciej Perkowski <[email protected]>
@PerMac PerMac force-pushed the patch/make_skip_reason_clearer branch from e763ef1 to e3540e8 Compare October 21, 2022 14:52
PerMac pushed a commit that referenced this pull request Jan 3, 2023
This patch reworks how fragments are handled in the net_buf
infrastructure.

In particular, it removes the union around the node and frags members in
the main net_buf structure. This is done so that both can be used at the
same time, at a cost of 4 bytes per net_buf instance.
This implies that the layout of net_buf instances changes whenever being
inserted into a queue (fifo or lifo) or a linked list (slist).

Until now, this is what happened when enqueueing a net_buf with frags in
a queue or linked list:

1.1 Before enqueueing:

 +--------+      +--------+      +--------+
 |#1  node|\     |#2  node|\     |#3  node|\
 |        | \    |        | \    |        | \
 | frags  |------| frags  |------| frags  |------NULL
 +--------+      +--------+      +--------+

net_buf #1 has 2 fragments, net_bufs #2 and #3. Both the node and frags
pointers (they are the same, since they are unioned) point to the next
fragment.

1.2 After enqueueing:

 +--------+      +--------+      +--------+      +--------+      +--------+
 |q/slist |------|#1  node|------|#2  node|------|#3  node|------|q/slist |
 |node    |      | *flag  | /    | *flag  | /    |        | /    |node    |
 |        |      | frags  |/     | frags  |/     | frags  |/     |        |
 +--------+      +--------+      +--------+      +--------+      +--------+

When enqueing a net_buf (in this case #1) that contains fragments, the
current net_buf implementation actually enqueues all the fragments (in
this case #2 and #3) as actual queue/slist items, since node and frags
are one and the same in memory. This makes the enqueuing operation
expensive and it makes it impossible to atomically dequeue. The `*flag`
notation here means that the `flags` member has been set to
`NET_BUF_FRAGS` in order to be able to reconstruct the frags pointers
when dequeuing.

After this patch, the layout changes considerably:

2.1 Before enqueueing:

 +--------+       +--------+       +--------+
 |#1  node|--NULL |#2  node|--NULL |#3  node|--NULL
 |        |       |        |       |        |
 | frags  |-------| frags  |-------| frags  |------NULL
 +--------+       +--------+       +--------+

This is very similar to 1.1, except that now node and frags are
different pointers, so node is just set to NULL.

2.2 After enqueueing:

 +--------+       +--------+       +--------+
 |q/slist |-------|#1  node|-------|q/slist |
 |node    |       |        |       |node    |
 |        |       | frags  |       |        |
 +--------+       +--------+       +--------+
                      |            +--------+       +--------+
                      |            |#2  node|--NULL |#3  node|--NULL
                      |            |        |       |        |
                      +------------| frags  |-------| frags  |------NULL
                                   +--------+       +--------+

When enqueuing net_buf #1, now we only enqueue that very item, instead
of enqueing the frags as well, since now node and frags are separate
pointers. This simplifies the operation and makes it atomic.

Resolves zephyrproject-rtos#52718.

Signed-off-by: Carles Cufi <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant