On 12/16/2021 3:08 AM, Ilpo Järvinen wrote:
> On Mon, 6 Dec 2021, Ricardo Martinez wrote:
>
...
>> +static struct cldma_request *t7xx_cldma_ring_step_forward(struct cldma_ring *ring,
>> + struct cldma_request *req)
>> +{
>> + if (req->entry.next == &ring->gpd_ring)
>> + return list_first_entry(&ring->gpd_ring, struct cldma_request, entry);
>> +
>> + return list_next_entry(req, entry);
>> +}
>> +
>> +static struct cldma_request *t7xx_cldma_ring_step_backward(struct cldma_ring *ring,
>> + struct cldma_request *req)
>> +{
>> + if (req->entry.prev == &ring->gpd_ring)
>> + return list_last_entry(&ring->gpd_ring, struct cldma_request, entry);
>> +
>> + return list_prev_entry(req, entry);
>> +}
> Wouldn't these two seems generic enough to warrant adding something like
> list_next/prev_entry_circular(...) to list.h?
Agree, in the upcoming version I'm planning to include something like
this to list.h as suggested:
#define list_next_entry_circular(pos, ptr, member) \
((pos)->member.next == (ptr) ? \
list_first_entry(ptr, typeof(*(pos)), member) : \
list_next_entry(pos, member))
On Tue, Jan 11, 2022 at 08:55:58PM -0800, Martinez, Ricardo wrote:
> On 12/16/2021 3:08 AM, Ilpo J?rvinen wrote:
> > On Mon, 6 Dec 2021, Ricardo Martinez wrote:
> > > + if (req->entry.next == &ring->gpd_ring)
> > > + return list_first_entry(&ring->gpd_ring, struct cldma_request, entry);
> > > +
> > > + return list_next_entry(req, entry);
...
> > > + if (req->entry.prev == &ring->gpd_ring)
> > > + return list_last_entry(&ring->gpd_ring, struct cldma_request, entry);
> > > +
> > > + return list_prev_entry(req, entry);
...
> > Wouldn't these two seems generic enough to warrant adding something like
> > list_next/prev_entry_circular(...) to list.h?
>
> Agree, in the upcoming version I'm planning to include something like this
> to list.h as suggested:
I think you mean for next and prev, i.o.w. two helpers, correct?
> #define list_next_entry_circular(pos, ptr, member) \
> ?? ?((pos)->member.next == (ptr) ? \
I believe this is list_entry_is_head().
> ?? ?list_first_entry(ptr, typeof(*(pos)), member) : \
> ?? ?list_next_entry(pos, member))
--
With Best Regards,
Andy Shevchenko
On Wed, 12 Jan 2022, Andy Shevchenko wrote:
> On Tue, Jan 11, 2022 at 08:55:58PM -0800, Martinez, Ricardo wrote:
> > On 12/16/2021 3:08 AM, Ilpo J?rvinen wrote:
> > > On Mon, 6 Dec 2021, Ricardo Martinez wrote:
>
> > > > + if (req->entry.next == &ring->gpd_ring)
> > > > + return list_first_entry(&ring->gpd_ring, struct cldma_request, entry);
> > > > +
> > > > + return list_next_entry(req, entry);
>
> ...
>
> > > > + if (req->entry.prev == &ring->gpd_ring)
> > > > + return list_last_entry(&ring->gpd_ring, struct cldma_request, entry);
> > > > +
> > > > + return list_prev_entry(req, entry);
>
> ...
>
> > > Wouldn't these two seems generic enough to warrant adding something like
> > > list_next/prev_entry_circular(...) to list.h?
> >
> > Agree, in the upcoming version I'm planning to include something like this
> > to list.h as suggested:
>
> I think you mean for next and prev, i.o.w. two helpers, correct?
>
> > #define list_next_entry_circular(pos, ptr, member) \
>
> > ?? ?((pos)->member.next == (ptr) ? \
>
> I believe this is list_entry_is_head().
It takes .next so it's not the same as list_entry_is_head() and
list_entry_is_last() doesn't exist.
--
i.
> > ?? ?list_first_entry(ptr, typeof(*(pos)), member) : \
> > ?? ?list_next_entry(pos, member))
>
>
On Wed, Jan 12, 2022 at 04:24:52PM +0200, Ilpo J?rvinen wrote:
> On Wed, 12 Jan 2022, Andy Shevchenko wrote:
> > On Tue, Jan 11, 2022 at 08:55:58PM -0800, Martinez, Ricardo wrote:
> > > On 12/16/2021 3:08 AM, Ilpo J?rvinen wrote:
> > > > On Mon, 6 Dec 2021, Ricardo Martinez wrote:
> >
> > > > > + if (req->entry.next == &ring->gpd_ring)
> > > > > + return list_first_entry(&ring->gpd_ring, struct cldma_request, entry);
> > > > > +
> > > > > + return list_next_entry(req, entry);
> >
> > ...
> >
> > > > > + if (req->entry.prev == &ring->gpd_ring)
> > > > > + return list_last_entry(&ring->gpd_ring, struct cldma_request, entry);
> > > > > +
> > > > > + return list_prev_entry(req, entry);
> >
> > ...
> >
> > > > Wouldn't these two seems generic enough to warrant adding something like
> > > > list_next/prev_entry_circular(...) to list.h?
> > >
> > > Agree, in the upcoming version I'm planning to include something like this
> > > to list.h as suggested:
> >
> > I think you mean for next and prev, i.o.w. two helpers, correct?
> >
> > > #define list_next_entry_circular(pos, ptr, member) \
> >
> > > ?? ?((pos)->member.next == (ptr) ? \
> >
> > I believe this is list_entry_is_head().
>
> It takes .next so it's not the same as list_entry_is_head() and
> list_entry_is_last() doesn't exist.
But we have list_last_entry(). So, what about
list_last_entry() == pos ? first : next;
and counterpart
list_first_entry() == pos ? last : prev;
?
> > > ?? ?list_first_entry(ptr, typeof(*(pos)), member) : \
> > > ?? ?list_next_entry(pos, member))
--
With Best Regards,
Andy Shevchenko
On Wed, 12 Jan 2022, Andy Shevchenko wrote:
> On Wed, Jan 12, 2022 at 04:24:52PM +0200, Ilpo J?rvinen wrote:
> > On Wed, 12 Jan 2022, Andy Shevchenko wrote:
> > > On Tue, Jan 11, 2022 at 08:55:58PM -0800, Martinez, Ricardo wrote:
> > > > On 12/16/2021 3:08 AM, Ilpo J?rvinen wrote:
> > > > > On Mon, 6 Dec 2021, Ricardo Martinez wrote:
> > >
> > > > > > + if (req->entry.next == &ring->gpd_ring)
> > > > > > + return list_first_entry(&ring->gpd_ring, struct cldma_request, entry);
> > > > > > +
> > > > > > + return list_next_entry(req, entry);
> > >
> > > ...
> > >
> > > > > > + if (req->entry.prev == &ring->gpd_ring)
> > > > > > + return list_last_entry(&ring->gpd_ring, struct cldma_request, entry);
> > > > > > +
> > > > > > + return list_prev_entry(req, entry);
> > >
> > > ...
> > >
> > > > > Wouldn't these two seems generic enough to warrant adding something like
> > > > > list_next/prev_entry_circular(...) to list.h?
> > > >
> > > > Agree, in the upcoming version I'm planning to include something like this
> > > > to list.h as suggested:
> > >
> > > I think you mean for next and prev, i.o.w. two helpers, correct?
> > >
> > > > #define list_next_entry_circular(pos, ptr, member) \
One thing I missed earlier, the sigrature should instead of ptr have head:
#define list_next_entry_circular(pos, head, member)
> > > > ?? ?((pos)->member.next == (ptr) ? \
> > >
> > > I believe this is list_entry_is_head().
> >
> > It takes .next so it's not the same as list_entry_is_head() and
> > list_entry_is_last() doesn't exist.
>
> But we have list_last_entry(). So, what about
>
> list_last_entry() == pos ? first : next;
>
> and counterpart
>
> list_first_entry() == pos ? last : prev;
>
> ?
Yes, although now that I think it more, using them implies the head
element has to be always accessed. It might be marginally cache friendlier
to use list_entry_is_head you originally suggested but get the next entry
first:
({
typeof(pos) next__ = list_next_entry(pos, member); \
!list_entry_is_head(next__, head, member) ? next__ : list_next_entry(next__, member);
})
(This was written directly to email, entirely untested).
Here, the head element would only get accessed when we really need to walk
through it.
> > > > ?? ?list_first_entry(ptr, typeof(*(pos)), member) : \
> > > > ?? ?list_next_entry(pos, member))
--
i.
On 1/12/2022 10:16 AM, Ilpo Järvinen wrote:
> On Wed, 12 Jan 2022, Andy Shevchenko wrote:
>
>> On Wed, Jan 12, 2022 at 04:24:52PM +0200, Ilpo Järvinen wrote:
>>> On Wed, 12 Jan 2022, Andy Shevchenko wrote:
>>>> On Tue, Jan 11, 2022 at 08:55:58PM -0800, Martinez, Ricardo wrote:
>>>>> On 12/16/2021 3:08 AM, Ilpo Järvinen wrote:
>>>>>> On Mon, 6 Dec 2021, Ricardo Martinez wrote:
>>>>>>> + if (req->entry.next == &ring->gpd_ring)
>>>>>>> + return list_first_entry(&ring->gpd_ring, struct cldma_request, entry);
>>>>>>> +
>>>>>>> + return list_next_entry(req, entry);
>>>> ...
>>>>
>>>>>>> + if (req->entry.prev == &ring->gpd_ring)
>>>>>>> + return list_last_entry(&ring->gpd_ring, struct cldma_request, entry);
>>>>>>> +
>>>>>>> + return list_prev_entry(req, entry);
>>>> ...
>>>>
>>>>>> Wouldn't these two seems generic enough to warrant adding something like
>>>>>> list_next/prev_entry_circular(...) to list.h?
>>>>> Agree, in the upcoming version I'm planning to include something like this
>>>>> to list.h as suggested:
>>>> I think you mean for next and prev, i.o.w. two helpers, correct?
>>>>
>>>>> #define list_next_entry_circular(pos, ptr, member) \
> One thing I missed earlier, the sigrature should instead of ptr have head:
> #define list_next_entry_circular(pos, head, member)
>
>>>>> ((pos)->member.next == (ptr) ? \
>>>> I believe this is list_entry_is_head().
>>> It takes .next so it's not the same as list_entry_is_head() and
>>> list_entry_is_last() doesn't exist.
>> But we have list_last_entry(). So, what about
>>
>> list_last_entry() == pos ? first : next;
>>
>> and counterpart
>>
>> list_first_entry() == pos ? last : prev;
>>
>> ?
> Yes, although now that I think it more, using them implies the head
> element has to be always accessed. It might be marginally cache friendlier
> to use list_entry_is_head you originally suggested but get the next entry
> first:
> ({
> typeof(pos) next__ = list_next_entry(pos, member); \
> !list_entry_is_head(next__, head, member) ? next__ : list_next_entry(next__, member);
> })
> (This was written directly to email, entirely untested).
>
> Here, the head element would only get accessed when we really need to walk
> through it.
I'm not sure if list_next_entry() will work for the last element, what
about using list_is_last()?
This way we avoid accessing head if not needed and does not to use
'container_of()' on (pos)->member.next.
(list_is_last(&(pos)->member, head) ? \
list_first_entry(head, typeof(*(pos)), member) : \
list_next_entry(pos, member))
(untested)
>>>>> list_first_entry(ptr, typeof(*(pos)), member) : \
>>>>> list_next_entry(pos, member))
On Wed, 12 Jan 2022, Martinez, Ricardo wrote:
>
> On 1/12/2022 10:16 AM, Ilpo Järvinen wrote:
> > On Wed, 12 Jan 2022, Andy Shevchenko wrote:
> >
> > > On Wed, Jan 12, 2022 at 04:24:52PM +0200, Ilpo Järvinen wrote:
> > > > On Wed, 12 Jan 2022, Andy Shevchenko wrote:
> > > > > On Tue, Jan 11, 2022 at 08:55:58PM -0800, Martinez, Ricardo wrote:
> > > > > > On 12/16/2021 3:08 AM, Ilpo Järvinen wrote:
> > > > > > > On Mon, 6 Dec 2021, Ricardo Martinez wrote:
> > > > > > > > + if (req->entry.next == &ring->gpd_ring)
> > > > > > > > + return list_first_entry(&ring->gpd_ring, struct
> > > > > > > > cldma_request, entry);
> > > > > > > > +
> > > > > > > > + return list_next_entry(req, entry);
> > > > > ...
> > > > >
> > > > > > > > + if (req->entry.prev == &ring->gpd_ring)
> > > > > > > > + return list_last_entry(&ring->gpd_ring, struct
> > > > > > > > cldma_request, entry);
> > > > > > > > +
> > > > > > > > + return list_prev_entry(req, entry);
> > > > > ...
> > > > >
> > > > > > > Wouldn't these two seems generic enough to warrant adding
> > > > > > > something like
> > > > > > > list_next/prev_entry_circular(...) to list.h?
> > > > > > Agree, in the upcoming version I'm planning to include something
> > > > > > like this
> > > > > > to list.h as suggested:
> > > > > I think you mean for next and prev, i.o.w. two helpers, correct?
> > > > >
> > > > > > #define list_next_entry_circular(pos, ptr, member) \
> > One thing I missed earlier, the sigrature should instead of ptr have head:
> > #define list_next_entry_circular(pos, head, member)
> >
> > > > > > ((pos)->member.next == (ptr) ? \
> > > > > I believe this is list_entry_is_head().
> > > > It takes .next so it's not the same as list_entry_is_head() and
> > > > list_entry_is_last() doesn't exist.
> > > But we have list_last_entry(). So, what about
> > >
> > > list_last_entry() == pos ? first : next;
> > >
> > > and counterpart
> > >
> > > list_first_entry() == pos ? last : prev;
> > >
> > > ?
> > Yes, although now that I think it more, using them implies the head
> > element has to be always accessed. It might be marginally cache friendlier
> > to use list_entry_is_head you originally suggested but get the next entry
> > first:
> > ({
> > typeof(pos) next__ = list_next_entry(pos, member); \
> > !list_entry_is_head(next__, head, member) ? next__ :
> > list_next_entry(next__, member);
> > })
> > (This was written directly to email, entirely untested).
> >
> > Here, the head element would only get accessed when we really need to walk
> > through it.
>
> I'm not sure if list_next_entry() will work for the last element, what about
> using list_is_last()?
Why wouldn't it? E.g., list_for_each_entry() does it for the last entry
before terminating the for loop.
--
i.
> This way we avoid accessing head if not needed and does not to use
> 'container_of()' on (pos)->member.next.
>
> (list_is_last(&(pos)->member, head) ? \
> list_first_entry(head, typeof(*(pos)), member) : \
> list_next_entry(pos, member))
>
> (untested)
>
> > > > > > list_first_entry(ptr, typeof(*(pos)), member) : \
> > > > > > list_next_entry(pos, member))
>
On 1/12/2022 11:24 AM, Ilpo Järvinen wrote:
> On Wed, 12 Jan 2022, Martinez, Ricardo wrote:
>
>> On 1/12/2022 10:16 AM, Ilpo Järvinen wrote:
>>> On Wed, 12 Jan 2022, Andy Shevchenko wrote:
>>>
>>>> On Wed, Jan 12, 2022 at 04:24:52PM +0200, Ilpo Järvinen wrote:
>>>>> On Wed, 12 Jan 2022, Andy Shevchenko wrote:
>>>>>> On Tue, Jan 11, 2022 at 08:55:58PM -0800, Martinez, Ricardo wrote:
>>>>>>> On 12/16/2021 3:08 AM, Ilpo Järvinen wrote:
>>>>>>>> On Mon, 6 Dec 2021, Ricardo Martinez wrote:
>>>>>>>>> + if (req->entry.next == &ring->gpd_ring)
>>>>>>>>> + return list_first_entry(&ring->gpd_ring, struct
>>>>>>>>> cldma_request, entry);
>>>>>>>>> +
>>>>>>>>> + return list_next_entry(req, entry);
>>>>>> ...
>>>>>>
>>>>>>>>> + if (req->entry.prev == &ring->gpd_ring)
>>>>>>>>> + return list_last_entry(&ring->gpd_ring, struct
>>>>>>>>> cldma_request, entry);
>>>>>>>>> +
>>>>>>>>> + return list_prev_entry(req, entry);
>>>>>> ...
>>>>>>
>>>>>>>> Wouldn't these two seems generic enough to warrant adding
>>>>>>>> something like
>>>>>>>> list_next/prev_entry_circular(...) to list.h?
>>>>>>> Agree, in the upcoming version I'm planning to include something
>>>>>>> like this
>>>>>>> to list.h as suggested:
>>>>>> I think you mean for next and prev, i.o.w. two helpers, correct?
>>>>>>
>>>>>>> #define list_next_entry_circular(pos, ptr, member) \
>>> One thing I missed earlier, the sigrature should instead of ptr have head:
>>> #define list_next_entry_circular(pos, head, member)
>>>
>>>>>>> ((pos)->member.next == (ptr) ? \
>>>>>> I believe this is list_entry_is_head().
>>>>> It takes .next so it's not the same as list_entry_is_head() and
>>>>> list_entry_is_last() doesn't exist.
>>>> But we have list_last_entry(). So, what about
>>>>
>>>> list_last_entry() == pos ? first : next;
>>>>
>>>> and counterpart
>>>>
>>>> list_first_entry() == pos ? last : prev;
>>>>
>>>> ?
>>> Yes, although now that I think it more, using them implies the head
>>> element has to be always accessed. It might be marginally cache friendlier
>>> to use list_entry_is_head you originally suggested but get the next entry
>>> first:
>>> ({
>>> typeof(pos) next__ = list_next_entry(pos, member); \
>>> !list_entry_is_head(next__, head, member) ? next__ :
>>> list_next_entry(next__, member);
>>> })
>>> (This was written directly to email, entirely untested).
>>>
>>> Here, the head element would only get accessed when we really need to walk
>>> through it.
>> I'm not sure if list_next_entry() will work for the last element, what about
>> using list_is_last()?
> Why wouldn't it? E.g., list_for_each_entry() does it for the last entry
> before terminating the for loop.
I wasn't sure about using container_of() on the head of the list, but I
see that it is not a problem.
Would that still be preferred over the list_is_last() approach?
On Wed, 12 Jan 2022, Martinez, Ricardo wrote:
>
> On 1/12/2022 11:24 AM, Ilpo J?rvinen wrote:
> > On Wed, 12 Jan 2022, Martinez, Ricardo wrote:
> >
> > > On 1/12/2022 10:16 AM, Ilpo J?rvinen wrote:
> > > > On Wed, 12 Jan 2022, Andy Shevchenko wrote:
> > > >
> > > > > On Wed, Jan 12, 2022 at 04:24:52PM +0200, Ilpo J?rvinen wrote:
> > > > > > On Wed, 12 Jan 2022, Andy Shevchenko wrote:
> > > > > > > On Tue, Jan 11, 2022 at 08:55:58PM -0800, Martinez, Ricardo wrote:
> > > > > > > > On 12/16/2021 3:08 AM, Ilpo J?rvinen wrote:
> > > > > > > > > On Mon, 6 Dec 2021, Ricardo Martinez wrote:
> > > > > > > > > > + if (req->entry.next == &ring->gpd_ring)
> > > > > > > > > > + return list_first_entry(&ring->gpd_ring,
> > > > > > > > > > struct
> > > > > > > > > > cldma_request, entry);
> > > > > > > > > > +
> > > > > > > > > > + return list_next_entry(req, entry);
> > > > > > > ...
> > > > > > >
> > > > > > > > > > + if (req->entry.prev == &ring->gpd_ring)
> > > > > > > > > > + return list_last_entry(&ring->gpd_ring, struct
> > > > > > > > > > cldma_request, entry);
> > > > > > > > > > +
> > > > > > > > > > + return list_prev_entry(req, entry);
> > > > > > > ...
> > > > > > >
> > > > > > > > > Wouldn't these two seems generic enough to warrant adding
> > > > > > > > > something like
> > > > > > > > > list_next/prev_entry_circular(...) to list.h?
> > > > > > > > Agree, in the upcoming version I'm planning to include something
> > > > > > > > like this
> > > > > > > > to list.h as suggested:
> > > > > > > I think you mean for next and prev, i.o.w. two helpers, correct?
> > > > > > >
> > > > > > > > #define list_next_entry_circular(pos, ptr, member) \
> > > > One thing I missed earlier, the sigrature should instead of ptr have
> > > > head:
> > > > #define list_next_entry_circular(pos, head, member)
> > > >
> > > > > > > > ?? ?((pos)->member.next == (ptr) ? \
> > > > > > > I believe this is list_entry_is_head().
> > > > > > It takes .next so it's not the same as list_entry_is_head() and
> > > > > > list_entry_is_last() doesn't exist.
> > > > > But we have list_last_entry(). So, what about
> > > > >
> > > > > list_last_entry() == pos ? first : next;
> > > > >
> > > > > and counterpart
> > > > >
> > > > > list_first_entry() == pos ? last : prev;
> > > > >
> > > > > ?
> > > > Yes, although now that I think it more, using them implies the head
> > > > element has to be always accessed. It might be marginally cache
> > > > friendlier
> > > > to use list_entry_is_head you originally suggested but get the next
> > > > entry
> > > > first:
> > > > ({
> > > > typeof(pos) next__ = list_next_entry(pos, member); \
> > > > !list_entry_is_head(next__, head, member) ? next__ :
> > > > list_next_entry(next__, member);
> > > > })
> > > > (This was written directly to email, entirely untested).
> > > >
> > > > Here, the head element would only get accessed when we really need to
> > > > walk
> > > > through it.
> > > I'm not sure if list_next_entry() will work for the last element, what
> > > about
> > > using list_is_last()?
> > Why wouldn't it? E.g., list_for_each_entry() does it for the last entry
> > before terminating the for loop.
>
> I wasn't sure about using container_of() on the head of the list, but I see
> that it is not a problem.
>
> Would that still be preferred over the list_is_last() approach?
I think list_is_last() is fine if that's what you want to use.
...I missed earlier the fact that you were referring to something else
than list_entry_is_last thanks to these n similarly named functions :-).
--
i.