Re: [patch] Real-Time Preemption, -RT-2.6.12-rc4-V0.7.47-06

Oleg Nesterov
2005-05-23T14:57:50Z

Ingo Molnar wrote:
>
> Changes:
>
> - more plist fixes (Daniel Walker)

plists were changed so that all nodes are tied via ->sp_node.

Now:

#define plist_for_each(pos1, head) \
list_for_each_entry(pos1, &((head)->sp_node), sp_node)

This is correct.

plist_for_each(curr1, &old_owner->pi_waiters) {
w = plist_entry(curr1, struct rt_mutex_waiter, pi_list);
if (w == waiter)
goto ok;
}

And this is not. because:

#define plist_entry(ptr, type, member) \
container_of(plist_first(ptr), type, member)
^^^^^^^^^^^
struct plist * plist_first(struct plist *plist)
{
return list_entry(plist->dp_node.next, struct plist, dp_node);
}

So the very first node will be skipped, iteration will be out of order,
and you will have the plist's *head* as a last element (which is not
struct rt_mutex_waiter, of course).

> unsigned plist_empty(const struct plist *plist)
> {
> - return list_empty (&plist->dp_node);
> + return list_empty(&plist->dp_node) && list_empty(&plist->sp_node);
> }

It's enough to check list_empty(&plist->sp_node) only.

And I don't understand why __plist_add_sorted is so complecated.
Why should we consider ->prio == INT_MAX as a special case?
This is also strange:

new_sp_head:
itr_pl2 = container_of(itr_pl->dp_node.prev, struct plist, dp_node);
list_add(&pl->sp_node, &itr_pl2->sp_node);

Why? Just list_add_tail(&pl->sp_node, itr_pl->sp_node), you don't
need itr_pl2 at all.

Daniel, if you accepted all-nodes-tied-via-sp_node idea, could you
also look at the code I've suggested. I think it is much simpler
and understandable.

void plist_add(struct plist *new, struct plist *head)
{
struct plist* pos;

INIT_LIST_HEAD(&new->dp_node);

list_for_each_entry(pos, &head->dp_node, dp_node)
if (new->prio < pos->prio)
goto lt_prio;
else if (new->prio == pos->prio) {
pos = list_entry(pos->dp_node.next,
struct plist, dp_node);
goto eq_prio;
}

lt_prio:
list_add_tail(&new->dp_node, &pos->dp_node);
eq_prio:
list_add_tail(&new->sp_node, &pos->sp_node);
}

void plist_del(struct plist *del)
{
if (!list_empty(&del->dp_node)) {
struct plist *next = list_entry(del->sp_node.next,
struct plist, sp_node);
list_move_tail(&next->dp_node, &del->dp_node);
list_del_init(&del->dp_node);
}

list_del_init(&del->sp_node);
}

Personally, I think it is better to have pl_head for plist's head,
and pl_node for nodes. It is pointless to store ->prio in the plist's
head, it can be found in plist_first()->prio. This way we can trim
the size of rt_mutex to 32 bytes, and it is good for typechecking.

Ingo, did you see these patches?
http://marc.theaimsgroup.com/?l=linux-kernel&m=111565001426673
http://marc.theaimsgroup.com/?l=linux-kernel&m=111565001415428
http://marc.theaimsgroup.com/?l=linux-kernel&m=111565001427334
http://marc.theaimsgroup.com/?l=linux-kernel&m=111565001408303
?

What do you think?

Oleg.