Re: [PATCH 1/4] mm/gup: add compound page list iterator

[John Hubbard <jhubbard@nvidia.com>]

On 2/4/21 11:53 AM, Jason Gunthorpe wrote:
> On Wed, Feb 03, 2021 at 03:00:01PM -0800, John Hubbard wrote:
>>> +static inline void compound_next(unsigned long i, unsigned long npages,
>>> +				 struct page **list, struct page **head,
>>> +				 unsigned int *ntails)
>>> +{
>>> +	if (i >= npages)
>>> +		return;
>>> +
>>> +	*ntails = count_ntails(list + i, npages - i);
>>> +	*head = compound_head(list[i]);
>>> +}
>>> +
>>> +#define for_each_compound_head(i, list, npages, head, ntails) \
>>
>> When using macros, which are dangerous in general, you have to worry about
>> things like name collisions. I really dislike that C has forced this unsafe
>> pattern upon us, but of course we are stuck with it, for iterator helpers.
>>
>> Given that we're stuck, you should probably use names such as __i, __list, etc,
>> in the the above #define. Otherwise you could stomp on existing variables.
> 
> Not this macro, it after cpp gets through with it all the macro names
> vanish, it can't collide with variables.
> 

Yes, I guess it does just vaporize, because it turns all the args into
their substituted values. I was just having flashbacks from similar cases
I guess.

> The usual worry is you might collide with other #defines, but we don't
> seem to worry about that in the kernel
> 

Well, I worry about it a little anyway. haha :)


thanks,
-- 
John Hubbard
NVIDIA