Message-Id: <6CBAC9B8-F675-41B8-AE72-C0A8B5FA0303@gmail.com>
Date:   Fri, 1 Apr 2022 17:16:29 +0200
From:   Jakob Koschel <jakobkoschel@...il.com>
To:     Dennis Dalessandro <dennis.dalessandro@...nelisnetworks.com>
Cc:     Mike Marciniszyn <mike.marciniszyn@...nelisnetworks.com>,
        Jason Gunthorpe <jgg@...pe.ca>, linux-rdma@...r.kernel.org,
        linux-kernel@...r.kernel.org, Mike Rapoport <rppt@...nel.org>,
        Brian Johannesmeyer <bjohannesmeyer@...il.com>,
        Cristiano Giuffrida <c.giuffrida@...nl>,
        "Bos, H.J." <h.j.bos@...nl>
Subject: Re: [PATCH] IB/hfi1: remove check of list iterator against head past
 the loop body



> On 1. Apr 2022, at 16:34, Dennis Dalessandro <dennis.dalessandro@...nelisnetworks.com> wrote:
> 
> On 3/31/22 6:45 PM, Jakob Koschel wrote:
>> When list_for_each_entry() completes the iteration over the whole list
>> without breaking the loop, the iterator value will be a bogus pointer
>> computed based on the head element.
>> 
>> While it is safe to use the pointer to determine if it was computed
>> based on the head element, either with list_entry_is_head() or
>> &pos->member == head, using the iterator variable after the loop should
>> be avoided.
>> 
>> In preparation to limit the scope of a list iterator to the list
>> traversal loop, use a dedicated pointer to point to the found element [1].
> 
> The code isn't searching the list. So there is no "found" element. It is walking
> a list of things and using each one it comes across.

Ok, I can see how this is confusing. I'll change the text accordingly.

> 
>> 
>> Link: https://lore.kernel.org/all/CAHk-=wgRr_D8CB-D9Kg-c=EHreAsk5SqXPwr9Y7k9sA6cWXJ6w@mail.gmail.com/ [1]
>> Signed-off-by: Jakob Koschel <jakobkoschel@...il.com>
>> ---
>> drivers/infiniband/hw/hfi1/tid_rdma.c | 16 +++++++++-------
>> 1 file changed, 9 insertions(+), 7 deletions(-)
>> 
>> diff --git a/drivers/infiniband/hw/hfi1/tid_rdma.c b/drivers/infiniband/hw/hfi1/tid_rdma.c
>> index 2a7abf7a1f7f..b12abf83a91c 100644
>> --- a/drivers/infiniband/hw/hfi1/tid_rdma.c
>> +++ b/drivers/infiniband/hw/hfi1/tid_rdma.c
>> @@ -1239,7 +1239,7 @@ static int kern_alloc_tids(struct tid_rdma_flow *flow)
>> 	struct hfi1_ctxtdata *rcd = flow->req->rcd;
>> 	struct hfi1_devdata *dd = rcd->dd;
>> 	u32 ngroups, pageidx = 0;
>> -	struct tid_group *group = NULL, *used;
>> +	struct tid_group *group = NULL, *used, *iter;
>> 	u8 use;
>> 
>> 	flow->tnode_cnt = 0;
>> @@ -1248,13 +1248,15 @@ static int kern_alloc_tids(struct tid_rdma_flow *flow)
>> 		goto used_list;
>> 
>> 	/* First look at complete groups */
>> -	list_for_each_entry(group, &rcd->tid_group_list.list, list) {
>> -		kern_add_tid_node(flow, rcd, "complete groups", group,
>> -				 group->size);
>> +	list_for_each_entry(iter, &rcd->tid_group_list.list, list) {
>> +		kern_add_tid_node(flow, rcd, "complete groups", iter,
>> +				 iter->size);
>> 
>> -		pageidx += group->size;
>> -		if (!--ngroups)
>> +		pageidx += iter->size;
>> +		if (!--ngroups) {
>> +			group = iter;
>> 			break;
>> +		}
>> 	}
> 
> The original code's intention was that if group is NULL we never iterated the
> list. If group != NULL we either iterated the entire list and ran out or we had
> enough and hit the break.

This is however not correct. list_for_each_entry() always sets 'group'.
It needs to do so to check the terminating condition of the loop.
It does so even if the list is empty and the loop body is not even executed once.

> 
> Under the proposed code, group is NULL if we never iterated the loop. It will
> also be NULL if we reach the end of the list. So the only time group != NULL is
> when we iterated the list and found all the groups we needed.
> 
>> 	if (pageidx >= flow->npagesets)
>> @@ -1277,7 +1279,7 @@ static int kern_alloc_tids(struct tid_rdma_flow *flow)
>> 	 * However, if we are at the head, we have reached the end of the
>> 	 * complete groups list from the first loop above
>> 	 */
>> -	if (group && &group->list == &rcd->tid_group_list.list)
>> +	if (!group)
> 
> So the problem here is group->list may point to gibberish if we iterated the
> entire loop?

In this case it would be &group->list == &rcd->tid_group_list.list
So the second part of the check is correct whereas the check "if group == true"
is just always true.

Since the intention is to move the scope of the list iterator into the macro
itself I'm removing the accesses to the list iterator after the loop body
('group' in this case).

> 
> Perhaps instead of group, add a bool, call it "need_more" or something. Set it
> to True at init time. Then when/if we hit the break set it to False. Means we
> found enough groups. Then down here we check if (need_more)....

If I understand you correctly the condition here should be:

	if (!list_empty(&rcd->tid_group_list.list) && !group)

which will only be true if the list actually did at least one iteration but did not
break early.

> 
> At the very least if you want to keep the code as it is, fix up the comments to
> make sense and explain the situation.

The code behaves exactly as it did before my patch.
Sorry I've missed that comment, I'll update it as well. As explain above, this
comment is a bit misleading since that's not actually what is executing and therefore
I got confused. group would be "at head" if the list being iterated
is empty, since the list iterator macros does not differentiate between
"not running any iteration of the loop since the list is empty" and 
"running to the end of the loop with at least one iteration".

> 
> -Denny

Sorry for the confusing terminology and missing the comment.

Thanks,
Jakob

Powered by blists - more mailing lists