Skip to content

dlist API enhancements supporting timer cleanup and scheduling fix #12485

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.

Sign up for GitHub

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

Closed
wants to merge 9 commits into from
Closed

dlist API enhancements supporting timer cleanup and scheduling fix #12485

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
882fe74
kernel: thread: correct type and member name of dlist node
pabigot Jan 4, 2019
9d57c54
kernel: sched: fix empty list detection
pabigot Jan 4, 2019
32f8c18
sys: dlist: Add sys_dnode_is_linked
pabigot Dec 30, 2018
681da23
kernel: timeout: remove local fix for double-remove
pabigot Dec 30, 2018
2e57052
kernel: poll: fix double-remove of node
pabigot Dec 30, 2018
fb0d742
kernel: timeout: detect inactive timeouts using dnode linked state
pabigot Jan 2, 2019
9416324
sys: dlist: add API to split lists
pabigot Jan 2, 2019
2d971c7
kernel: timeout: avoid premature adjustment of timeout
pabigot Jan 7, 2019
2e0d00b
tests: kernel: timer: test delay for schedule in timer callback
pabigot Jan 9, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion include/kernel.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -389,7 +389,7 @@ struct _thread_base {

/* this thread's entry in a ready/wait queue */
union {
sys_dlist_t qnode_dlist;
sys_dnode_t qnode_dnode;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Minor point, but if you're going to correct naming, it should remain "qnode_dlist" or become "qnode_list" or something. It's the node field that goes into a list, to distinguish it from the equivalent field that goes into an rbtree. There's little value in trying to encode the type of an object in its field name.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You can bring this up when I re-open #12248 and it shows up there, but basically---no, it's not encoding the type in the name, it's encoding the value's role.

The whole problem is the original name results in passing something named qnode_dlist in a position where the API requires a sys_dnode_t instance. It looks weird and is confusing. The field should not claim to be a list when it's a node. The list is somewhere else.

Copy link
Collaborator

Choose a reason for hiding this comment

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

no, it's not encoding the type in the name, it's encoding the value's role.

Peter, please don't start this fight. Variable names don't uniformly encode what a value "is" and never have, that's what compiler typesafety is for (which fell down here because of a design glitch with dlist -- those are typedefs and not separate structs, but that's a digression). Names tell you what it does.

The type was wrong, that's a bug that you fixed. The name was fine, and correctly indicates the intent of the author whether or not you personally agree with the convention in use. It's not productive (or even feasible) to run around giant codebases "fixing" stuff like other people's variable naming, no one ever agrees with that kind of thing and it just makes a mess. Some fights just aren't worth having.

struct rbnode qnode_rb;
};

Expand Down
2 changes: 1 addition & 1 deletion kernel/include/wait_q.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ static inline struct k_thread *_waitq_head(_wait_q_t *w)

#define _WAIT_Q_FOR_EACH(wq, thread_ptr) \
SYS_DLIST_FOR_EACH_CONTAINER(&((wq)->waitq), thread_ptr, \
base.qnode_dlist)
base.qnode_dnode)

static inline void _waitq_init(_wait_q_t *w)
{
Expand Down
2 changes: 1 addition & 1 deletion kernel/pipes.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -369,7 +369,7 @@ static bool pipe_xfer_prepare(sys_dlist_t *xfer_list,
* Add it to the transfer list.
*/
_unpend_thread(thread);
sys_dlist_append(xfer_list, &thread->base.qnode_dlist);
sys_dlist_append(xfer_list, &thread->base.qnode_dnode);
}

*waiter = (num_bytes > bytes_to_xfer) ? thread : NULL;
Expand Down
18 changes: 9 additions & 9 deletions kernel/sched.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -571,29 +571,29 @@ void _priq_dumb_add(sys_dlist_t *pq, struct k_thread *thread)

__ASSERT_NO_MSG(!_is_idle(thread));

SYS_DLIST_FOR_EACH_CONTAINER(pq, t, base.qnode_dlist) {
SYS_DLIST_FOR_EACH_CONTAINER(pq, t, base.qnode_dnode) {
if (_is_t1_higher_prio_than_t2(thread, t)) {
sys_dlist_insert_before(pq, &t->base.qnode_dlist,
&thread->base.qnode_dlist);
sys_dlist_insert_before(pq, &t->base.qnode_dnode,
&thread->base.qnode_dnode);
return;
}
}

sys_dlist_append(pq, &thread->base.qnode_dlist);
sys_dlist_append(pq, &thread->base.qnode_dnode);
}

void _priq_dumb_remove(sys_dlist_t *pq, struct k_thread *thread)
{
__ASSERT_NO_MSG(!_is_idle(thread));

sys_dlist_remove(&thread->base.qnode_dlist);
sys_dlist_remove(&thread->base.qnode_dnode);
}

struct k_thread *_priq_dumb_best(sys_dlist_t *pq)
{
sys_dnode_t *n = sys_dlist_peek_head(pq);

return CONTAINER_OF(n, struct k_thread, base.qnode_dlist);
return CONTAINER_OF(n, struct k_thread, base.qnode_dnode);
}

bool _priq_rb_lessthan(struct rbnode *a, struct rbnode *b)
Expand Down Expand Up @@ -663,15 +663,15 @@ void _priq_mq_add(struct _priq_mq *pq, struct k_thread *thread)
{
int priority_bit = thread->base.prio - K_HIGHEST_THREAD_PRIO;

sys_dlist_append(&pq->queues[priority_bit], &thread->base.qnode_dlist);
sys_dlist_append(&pq->queues[priority_bit], &thread->base.qnode_dnode);
pq->bitmask |= (1 << priority_bit);
}

void _priq_mq_remove(struct _priq_mq *pq, struct k_thread *thread)
{
int priority_bit = thread->base.prio - K_HIGHEST_THREAD_PRIO;

sys_dlist_remove(&thread->base.qnode_dlist);
sys_dlist_remove(&thread->base.qnode_dnode);
if (sys_dlist_is_empty(&pq->queues[priority_bit])) {
pq->bitmask &= ~(1 << priority_bit);
}
Expand All @@ -686,7 +686,7 @@ struct k_thread *_priq_mq_best(struct _priq_mq *pq)
sys_dlist_t *l = &pq->queues[__builtin_ctz(pq->bitmask)];
sys_dnode_t *n = sys_dlist_peek_head(l);

return CONTAINER_OF(n, struct k_thread, base.qnode_dlist);
return CONTAINER_OF(n, struct k_thread, base.qnode_dnode);
}

int _unpend_all(_wait_q_t *wait_q)
Expand Down