Remove redundant % size from queue APIs
#114
Conversation
include/sddf/network/queue.h
Outdated
| @@ -85,7 +85,7 @@ static inline bool net_queue_empty_active(net_queue_handle_t *queue) | |||
| */ | |||
| static inline bool net_queue_full_free(net_queue_handle_t *queue) | |||
| { | |||
| return !((queue->free->tail + 1 - queue->free->head) % queue->size); | |||
| return !(queue->free->tail + 1 - queue->free->head); | |||
There was a problem hiding this comment.
Both these changes to the full conditions are incorrect.
Firstly, remember that I haven't yet corrected the issue raised before that if the queue size is S, we currently only use S-1 entries. (This is next on my list of issues to fix.)
Bearing this in mind, the idea with this function is to catch the case when the tail is S-1 greater than the head. E.g.
tail = head + S - 1
Re-arranging this we get:
tail + 1 - head = S
which satisfies the old condition of:
!((tail + 1 - head) % S)
What you have changed it to instead is:
tail + 1 - head != 0, or tail + 1 != head
which means that a queue is only considered full when the tail is UINT16_MAX - 1 greater than the head (much larger than most of our queues :-) ).
I believe the correct condition should be
tail + 1 - head = size
There was a problem hiding this comment.
Yes, my bad. I will fix that.
Ivan-Velickovic
force-pushed
the
queue_remove_modulo
branch
2 times, most recently
from
f1b04e4 to
7afc667
Compare
| @@ -132,7 +132,7 @@ static inline bool blk_resp_queue_empty(blk_queue_handle_t *h) | |||
| */ | |||
| static inline bool blk_req_queue_full(blk_queue_handle_t *h) | |||
| { | |||
| return !((h->req_queue->head - h->req_queue->tail + 1) % h->queue_size); | |||
| return h->req_queue->head - h->req_queue->tail + 1 == h->req_queue->size; | |||
There was a problem hiding this comment.
I assume that blk seems to use head/tail in the opposite way to i2c/network/serial?
There was a problem hiding this comment.
Yes, I noticed that and made a comment #78 (comment).
There was a problem hiding this comment.
Maybe @erichchan999 can fix that or I can make another PR later.
There was a problem hiding this comment.
Yep happy to make a separate PR once this is merged
|
I'll do a benchmark run tomorrow morning and then merge (just to double check the change). Thanks for noticing my mistake @Courtney3141. |
Signed-off-by: Ivan Velickovic <[email protected]>
Signed-off-by: Ivan Velickovic <[email protected]>
Signed-off-by: Ivan Velickovic <[email protected]>
Courtney3141
force-pushed
the
queue_remove_modulo
branch
from
7afc667 to
4661da8
Compare
|
Since you will be away for a while, I re-ran a benchmark for you to check that performance is preserved and everything works as usual. I will merge this now. |
Closes #111.
Brought up by @isubasinghe.