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

[John Hubbard]

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