[URGENT] Queue allocation is broken after update to 11.0.1

[neboskreb]

[URGENT] This issue breaks an essential functionality.

Description

Queue allocation is broken after update to 11.0.1. Queues bigger than just few elements cannot be created.

The issue was introduced upstream by this commit to FreeRTOS-Kernel

Reproducing

Environment:

• Arduino_FreeRTOS Version: 11.0.1, 11.1.0 (current)
• Target platform: ATMega2560
• Host platform: Intel x64

Expected behavior

A queue for 64 items of 4 bytes each should be created:

QueueHandle_t result = xQueueGenericCreate(64, 4, queueQUEUE_TYPE_BASE);


Actual behavior

The operation fails internally, and the call returns NULL.

A smaller queue, e.g. 10 elements of 2 bytes, is created successfully.

Minimal reproducible example

Given proper type definitions, the issue is reproduced on a host platform Intel x64 as well. Test file for GTest is attached.

Analysis

The upstream commit which introduced the issue has added a cast to UBaseType_t in the parameter validation block.

The fault is caused by the size of UBaseType_t on AVR which is uint8_t, meaning its maximum value is 255. This reduces the max "valid" size to a very small number.

Bigger CPUs like SAM/ARM which have UBaseType_t defined as uint16_t/uint32_t do not suffer this issue because such a large queues are not allocated in practice. Therefore, this issue is unlikely to be noticed in the upstream repo.

Possible solutions

A) Remove only the broken cast at line 516
B) Revert the entire commit which introduced the issue
C) Remove the parameter validation block

I would recommend solution C. Let's discuss pros and contras in the comments section.

Action points

I shall draft a PR once we agree on the solution.

[feilipu]

Is this still relevant in version 11.1.0 ?

If so (and I presume it is) then this is a FreeRTOS kernel issue that should be fixed upstream, as it affects all 8-bit CPU ports. I suggest reporting it there, and referring to this issue as an anchor.

I agree it is a problem, and that @aggarg might have overlooked it.

[neboskreb]

Yes, the issue is still present in 11.1.0.

I don't think the guys upstream even aware of this issue because their CPUs are much more capable than ours. I'm afraid we have to fix it here, and hopefully we can push it upwards to rectify the cause.

[feilipu]

Usually @RichardBarry and @aggarg are very responsive to real concerns, particularly where they break existing support. Let's give them a moment.

[aggarg]

Thank you for pointing this out. Does this PR address the issue you are facing - FreeRTOS/FreeRTOS-Kernel#1153?

[feilipu]

@neboskreb Please confirm this works in your test. I'm AFK, and have no test machine.

If it is ok, please do a PR with identical solution with suggested close for this issue, or I can just add the commit and close. Let me know which, please.

[neboskreb]

@aggarg , @feilipu

Yes, it works.

[neboskreb]

@feilipu
Regarding the solution for AVR, let's have a quick round of discussion before we go forward.

My concern is that in this parameter validation block a division operation is used with an arbitrary divisor. This results in big performance impact in AVR, which does not have a hardware div instruction. In case of constant divisor, the compiler can convert a true division into a series of integer multiplications, which is a lesser evil; in the case of arbitrary divisor, e.g. which passed as parameter, such optimisation is not possible. The disassembly shows calling a subroutine for software division, which consumes around 200 cycles.

Though parameter validation improves robustness a lot, 200 cycles looks to me an overkill. I suggest removing this validation block altogether:

What do you think?

[feilipu]

I don't like division operations, in any respect. Particularly now I work mostly with 8085/z80/z180 which is about 20x slower than AVR in cycles.

But, I also don't like diverging from the upstream unnecessarily. Because I have to watch for diffs when updating code, forever.

So on balance, I'd support removing the check, if a clean cutout PR can be made (but not if there are many lines that need to be modified and maintained to make it work).

I'm unable to do tests now though. So if you want to propose a PR, then please test it appropriately.

[neboskreb]

Alignment with the upstream is a very strong point, indeed. Give me a minute to make a PR with the smallest possible footprint, and let's see then.

[feilipu]

Though I'm also thinking that queue creation is not particularly likely to be in performance critical code, but rather just during set-up.

Perhaps there's no real world win by avoiding the check?

Is there a particular use case where queue creation and deletion might occur often, where performance matters?

[neboskreb]

Not that I have such cases in my projects, indeed. You're probably right that this divergence from the upstream isn't that much necessary. I pushed a PR #137 which copies the upstream identically, so git should have no problem with the later update.

If we still want to optimise, there is an alternative PR #138. The change indeed overlaps the change in the upstream, so you will have to resolve it during the later update.

Please review both and approve one, kill the other :)