[iptables PATCH] list: fix prefetch dummy

[iptables PATCH] list: fix prefetch dummy

[Arturo Borrero Gonzalez]

linux_list.h:381:59: warning: right-hand operand of comma expression has no effect [-Wunused-value]
  for (pos = list_entry((head)->next, typeof(*pos), member), \
                                                           ^
libiptc.c:552:2: note: in expansion of macro 'list_for_each_entry'
  list_for_each_entry(c, &h->chains, list) {
  ^

[ Patch copied from one similar of Patrick McHardy on libnftnl ]

Signed-off-by: Arturo Borrero Gonzalez <arturo.borrero.glez@gmail.com>
---
 libiptc/linux_list.h |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/libiptc/linux_list.h b/libiptc/linux_list.h
index abdcf88..559e33c 100644
--- a/libiptc/linux_list.h
+++ b/libiptc/linux_list.h
@@ -27,7 +27,7 @@
 	1; \
 })
 
-#define prefetch(x)		1
+#define prefetch(x)		((void)0)
 
 /* empty define to make this work in userspace -HW */
 #define smp_wmb()

Re: [iptables PATCH] list: fix prefetch dummy

[Alexander Duyck]

On 04/06/2015 11:05 AM, Arturo Borrero Gonzalez wrote:
> linux_list.h:381:59: warning: right-hand operand of comma expression has no effect [-Wunused-value]
>   for (pos = list_entry((head)->next, typeof(*pos), member), \
>                                                            ^
> libiptc.c:552:2: note: in expansion of macro 'list_for_each_entry'
>   list_for_each_entry(c, &h->chains, list) {
>   ^
>
> [ Patch copied from one similar of Patrick McHardy on libnftnl ]
>
> Signed-off-by: Arturo Borrero Gonzalez <arturo.borrero.glez@gmail.com>
> ---
>  libiptc/linux_list.h |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/libiptc/linux_list.h b/libiptc/linux_list.h
> index abdcf88..559e33c 100644
> --- a/libiptc/linux_list.h
> +++ b/libiptc/linux_list.h
> @@ -27,7 +27,7 @@
>  	1; \
>  })
>  
> -#define prefetch(x)		1
> +#define prefetch(x)		((void)0)
>  
>  /* empty define to make this work in userspace -HW */
>  #define smp_wmb()
>

Why not just use "do {} while (0)"?  I know that is what is used in the
kernel for functions that don't do anything.

- Alex

Re: [iptables PATCH] list: fix prefetch dummy

[Jan Engelhardt]

On Tuesday 2015-04-07 02:38, Alexander Duyck wrote:

>On 04/06/2015 11:05 AM, Arturo Borrero Gonzalez wrote:
>> linux_list.h:381:59: warning: right-hand operand of comma expression has no effect [-Wunused-value]
>>   for (pos = list_entry((head)->next, typeof(*pos), member), \
>>                                                            ^
>> libiptc.c:552:2: note: in expansion of macro 'list_for_each_entry'
>>   list_for_each_entry(c, &h->chains, list) {
>>   ^
>>
>> [ Patch copied from one similar of Patrick McHardy on libnftnl ]
>>
>> Signed-off-by: Arturo Borrero Gonzalez <arturo.borrero.glez@gmail.com>
>> ---
>>  libiptc/linux_list.h |    2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/libiptc/linux_list.h b/libiptc/linux_list.h
>> index abdcf88..559e33c 100644
>> --- a/libiptc/linux_list.h
>> +++ b/libiptc/linux_list.h
>> @@ -27,7 +27,7 @@
>>  	1; \
>>  })
>>  
>> -#define prefetch(x)		1
>> +#define prefetch(x)		((void)0)
>>  
>>  /* empty define to make this work in userspace -HW */
>>  #define smp_wmb()
>>
>
>Why not just use "do {} while (0)"?  I know that is what is used in the
>kernel for functions that don't do anything.

I may be getting the terms wrong, but:
do{}while(0) is not an expression, it is a (block) control statement.
In particular, do{}while(0) won't evaluate to an rvalue.

Re: [iptables PATCH] list: fix prefetch dummy

[Alexander Duyck]

On 04/07/2015 01:37 AM, Jan Engelhardt wrote:
> On Tuesday 2015-04-07 02:38, Alexander Duyck wrote:
>
>> On 04/06/2015 11:05 AM, Arturo Borrero Gonzalez wrote:
>>> linux_list.h:381:59: warning: right-hand operand of comma expression has no effect [-Wunused-value]
>>>    for (pos = list_entry((head)->next, typeof(*pos), member), \
>>>                                                             ^
>>> libiptc.c:552:2: note: in expansion of macro 'list_for_each_entry'
>>>    list_for_each_entry(c, &h->chains, list) {
>>>    ^
>>>
>>> [ Patch copied from one similar of Patrick McHardy on libnftnl ]
>>>
>>> Signed-off-by: Arturo Borrero Gonzalez <arturo.borrero.glez@gmail.com>
>>> ---
>>>   libiptc/linux_list.h |    2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/libiptc/linux_list.h b/libiptc/linux_list.h
>>> index abdcf88..559e33c 100644
>>> --- a/libiptc/linux_list.h
>>> +++ b/libiptc/linux_list.h
>>> @@ -27,7 +27,7 @@
>>>   	1; \
>>>   })
>>>   
>>> -#define prefetch(x)		1
>>> +#define prefetch(x)		((void)0)
>>>   
>>>   /* empty define to make this work in userspace -HW */
>>>   #define smp_wmb()
>>>
>> Why not just use "do {} while (0)"?  I know that is what is used in the
>> kernel for functions that don't do anything.
> I may be getting the terms wrong, but:
> do{}while(0) is not an expression, it is a (block) control statement.
> In particular, do{}while(0) won't evaluate to an rvalue.

Right.  That is the point in this case.  I am assuming what Arturo is 
trying to accomplish since you shouldn't be able to evaluate ((void)0) 
as an rvalue either.

The prefetch(x) is an empty statement which is being cast as a void to 
avoid a compiler warning, so it falls into the first case as defined in: 
http://kernelnewbies.org/FAQ/DoWhile0

- Alex

Re: [iptables PATCH] list: fix prefetch dummy

[Jan Engelhardt]

On Tuesday 2015-04-07 19:45, Alexander Duyck wrote:
>>>>  -#define prefetch(x)		1
>>>> +#define prefetch(x)		((void)0)
>>>>
>>> Why not just use "do {} while (0)"?  I know that is what is used in the
>>> kernel for functions that don't do anything.
>> I may be getting the terms wrong, but:
>> do{}while(0) is not an expression, it is a (block) control statement.
>> In particular, do{}while(0) won't evaluate to an rvalue.
>
> Right.  That is the point in this case.  I am assuming what Arturo is trying to
> accomplish since you shouldn't be able to evaluate ((void)0) as an rvalue
> either.

The difference is that

	int i;
	i = 1, (void)0;

compiles, while

	i = 1, do{}while(0);

will not. Even less so with

	int i = 1, (void)0 vs
	int i = 1, do{}while(0);

[that looks so obscure that even I need to go figure out why that is
even permitted at this point].

Re: [iptables PATCH] list: fix prefetch dummy

[Alexander Duyck]

On 04/07/2015 11:17 AM, Jan Engelhardt wrote:
>
> On Tuesday 2015-04-07 19:45, Alexander Duyck wrote:
>>>>>   -#define prefetch(x)		1
>>>>> +#define prefetch(x)		((void)0)
>>>>>
>>>> Why not just use "do {} while (0)"?  I know that is what is used in the
>>>> kernel for functions that don't do anything.
>>> I may be getting the terms wrong, but:
>>> do{}while(0) is not an expression, it is a (block) control statement.
>>> In particular, do{}while(0) won't evaluate to an rvalue.
>>
>> Right.  That is the point in this case.  I am assuming what Arturo is trying to
>> accomplish since you shouldn't be able to evaluate ((void)0) as an rvalue
>> either.
>
> The difference is that
>
> 	int i;
> 	i = 1, (void)0;
>
> compiles, while
>
> 	i = 1, do{}while(0);
>

Okay, that explains it.  It is being used inside the init, condition, 
and post process sections of a for loop.  Thanks for explaining it.

- Alex

Re: [iptables PATCH] list: fix prefetch dummy

[Pablo Neira Ayuso]

On Mon, Apr 06, 2015 at 08:05:41PM +0200, Arturo Borrero Gonzalez wrote:
> linux_list.h:381:59: warning: right-hand operand of comma expression has no effect [-Wunused-value]
>   for (pos = list_entry((head)->next, typeof(*pos), member), \
>                                                            ^
> libiptc.c:552:2: note: in expansion of macro 'list_for_each_entry'
>   list_for_each_entry(c, &h->chains, list) {
>   ^
> 
> [ Patch copied from one similar of Patrick McHardy on libnftnl ]

Also applied, thanks.

Re: [iptables PATCH] list: fix prefetch dummy

[Stephen Hemminger]

On Mon, 06 Apr 2015 20:05:41 +0200
Arturo Borrero Gonzalez <arturo.borrero.glez@gmail.com> wrote:

> linux_list.h:381:59: warning: right-hand operand of comma expression has no effect [-Wunused-value]
>   for (pos = list_entry((head)->next, typeof(*pos), member), \
>                                                            ^
> libiptc.c:552:2: note: in expansion of macro 'list_for_each_entry'
>   list_for_each_entry(c, &h->chains, list) {
>   ^
> 
> [ Patch copied from one similar of Patrick McHardy on libnftnl ]
> 
> Signed-off-by: Arturo Borrero Gonzalez <arturo.borrero.glez@gmail.com>
> ---
>  libiptc/linux_list.h |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/libiptc/linux_list.h b/libiptc/linux_list.h
> index abdcf88..559e33c 100644
> --- a/libiptc/linux_list.h
> +++ b/libiptc/linux_list.h
> @@ -27,7 +27,7 @@
>  	1; \
>  })
>  
> -#define prefetch(x)		1
> +#define prefetch(x)		((void)0)
>  
>  /* empty define to make this work in userspace -HW */
>  #define smp_wmb()
> 
> --
> To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

The kernel dropped the prefetch() in the list macros a couple
of years ago. The prefetch() had no performance gain and hurt
in several cases.

Re: [iptables PATCH] list: fix prefetch dummy

[Pablo Neira Ayuso]

On Wed, Apr 08, 2015 at 05:18:25PM -0700, Stephen Hemminger wrote:
> On Mon, 06 Apr 2015 20:05:41 +0200
> Arturo Borrero Gonzalez <arturo.borrero.glez@gmail.com> wrote:
> 
> > linux_list.h:381:59: warning: right-hand operand of comma expression has no effect [-Wunused-value]
> >   for (pos = list_entry((head)->next, typeof(*pos), member), \
> >                                                            ^
> > libiptc.c:552:2: note: in expansion of macro 'list_for_each_entry'
> >   list_for_each_entry(c, &h->chains, list) {
> >   ^
> > 
> > [ Patch copied from one similar of Patrick McHardy on libnftnl ]
> > 
> > Signed-off-by: Arturo Borrero Gonzalez <arturo.borrero.glez@gmail.com>
> > ---
> >  libiptc/linux_list.h |    2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/libiptc/linux_list.h b/libiptc/linux_list.h
> > index abdcf88..559e33c 100644
> > --- a/libiptc/linux_list.h
> > +++ b/libiptc/linux_list.h
> > @@ -27,7 +27,7 @@
> >  	1; \
> >  })
> >  
> > -#define prefetch(x)		1
> > +#define prefetch(x)		((void)0)
> >  
> >  /* empty define to make this work in userspace -HW */
> >  #define smp_wmb()
> > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> The kernel dropped the prefetch() in the list macros a couple
> of years ago. The prefetch() had no performance gain and hurt
> in several cases.

We have an old copy of linux's list.h in our userspace trees. We can
probably refresh those to get them in sync with current at some point.