[Qemu-devel] [PATCH 1/2] docs/rcu: Distinguish rcu_dereference and atomic_rcu_read

[Qemu-devel] [PATCH 1/2] docs/rcu: Distinguish rcu_dereference and atomic_rcu_read

[Pranith Kumar]

Signed-off-by: Pranith Kumar <bobby.prani@gmail.com>
---
 docs/rcu.txt | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/docs/rcu.txt b/docs/rcu.txt
index c84e7f4..c177dcb 100644
--- a/docs/rcu.txt
+++ b/docs/rcu.txt
@@ -197,7 +197,9 @@ DIFFERENCES WITH LINUX
   critical section to become an updater.
 
 - atomic_rcu_read and atomic_rcu_set replace rcu_dereference and
-  rcu_assign_pointer.  They take a _pointer_ to the variable being accessed.
+  rcu_assign_pointer.  Note that although both atomic_rcu_read and
+  rcu_dereference take a _pointer_ to the variable being accessed,
+  atomic_rcu_read dereferences the pointer whereas rcu_dereference does not.
 
 - call_rcu is a macro that has an extra argument (the name of the first
   field in the struct, which must be a struct rcu_head), and expects the
-- 
2.10.1

[Qemu-devel] [PATCH 2/2] translate-all: Use proper type

[Pranith Kumar]

gcc does not warn about the wrong type since it is a void pointer
which can be cast to any type.

Signed-off-by: Pranith Kumar <bobby.prani@gmail.com>
---
 translate-all.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/translate-all.c b/translate-all.c
index 8ca393c..c77470a 100644
--- a/translate-all.c
+++ b/translate-all.c
@@ -412,7 +412,7 @@ static PageDesc *page_find_alloc(tb_page_addr_t index, int alloc)
 
     /* Level 2..N-1.  */
     for (i = V_L1_SHIFT / V_L2_BITS - 1; i > 0; i--) {
-        void **p = atomic_rcu_read(lp);
+        void *p = atomic_rcu_read(lp);
 
         if (p == NULL) {
             if (!alloc) {
-- 
2.10.1

Re: [Qemu-devel] [PATCH 2/2] translate-all: Use proper type

[Emilio G. Cota]

On Tue, Oct 18, 2016 at 10:56:20 -0400, Pranith Kumar wrote:
> gcc does not warn about the wrong type since it is a void pointer
> which can be cast to any type.
> 
> Signed-off-by: Pranith Kumar <bobby.prani@gmail.com>
> ---
>  translate-all.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/translate-all.c b/translate-all.c
> index 8ca393c..c77470a 100644
> --- a/translate-all.c
> +++ b/translate-all.c
> @@ -412,7 +412,7 @@ static PageDesc *page_find_alloc(tb_page_addr_t index, int alloc)
>  
>      /* Level 2..N-1.  */
>      for (i = V_L1_SHIFT / V_L2_BITS - 1; i > 0; i--) {
> -        void **p = atomic_rcu_read(lp);
> +        void *p = atomic_rcu_read(lp);
>  
>          if (p == NULL) {
>              if (!alloc) {

Let me redo your patch with more context (for patches like this using
format-patch -U<n> is useful):

$ git diff -U11 translate-all.c
diff --git a/translate-all.c b/translate-all.c
index 4200869..6928ace 100644
--- a/translate-all.c
+++ b/translate-all.c
@@ -405,23 +405,23 @@ static void page_init(void)
 static PageDesc *page_find_alloc(tb_page_addr_t index, int alloc)
 {
     PageDesc *pd;
     void **lp;
     int i;

     /* Level 1.  Always allocated.  */
     lp = l1_map + ((index >> V_L1_SHIFT) & (V_L1_SIZE - 1));

     /* Level 2..N-1.  */
     for (i = V_L1_SHIFT / V_L2_BITS - 1; i > 0; i--) {
-        void **p = atomic_rcu_read(lp);
+        void *p = atomic_rcu_read(lp);

         if (p == NULL) {
             if (!alloc) {
                 return NULL;
             }
             p = g_new0(void *, V_L2_SIZE);
             atomic_rcu_set(lp, p);
         }

         lp = p + ((index >> (i * V_L2_BITS)) & (V_L2_SIZE - 1));
     }

I prefer void **p since that matches lp's and l1_map's type.

It's true that since we're dealing with void * the compiler won't
complain either way.

		Emilio

Re: [Qemu-devel] [PATCH 2/2] translate-all: Use proper type

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 2184 bytes --]

On 10/18/2016 01:34 PM, Emilio G. Cota wrote:
> +++ b/translate-all.c
> @@ -405,23 +405,23 @@ static void page_init(void)
>  static PageDesc *page_find_alloc(tb_page_addr_t index, int alloc)
>  {
>      PageDesc *pd;
>      void **lp;
>      int i;
> 
>      /* Level 1.  Always allocated.  */
>      lp = l1_map + ((index >> V_L1_SHIFT) & (V_L1_SIZE - 1));
> 
>      /* Level 2..N-1.  */
>      for (i = V_L1_SHIFT / V_L2_BITS - 1; i > 0; i--) {
> -        void **p = atomic_rcu_read(lp);
> +        void *p = atomic_rcu_read(lp);
> 
>          if (p == NULL) {
>              if (!alloc) {
>                  return NULL;
>              }
>              p = g_new0(void *, V_L2_SIZE);
>              atomic_rcu_set(lp, p);
>          }
> 
>          lp = p + ((index >> (i * V_L2_BITS)) & (V_L2_SIZE - 1));

Pointer addition of 'void *' plus an offset is undefined (gcc, and
presumably clang, have an extension that treats it the same as computing
an offset to a 'char *'; but some compilers choke); this is because
sizeof(void) is unknown, so you don't know what stride to make for each
offset.  Or put another way, 'p + offset' is the same as
'&((*p)[offset])', but (*p)[offset] is ill-defined when p is the opaque
type void.

Pointer addition of 'void **' plus an offset is well-defined, because
sizeof(void*) is well-defined and therefore the stride (4 or 8) makes
sense.  Or in array notation, computing '&((*p)[offset]) means we are
skipping to the offset array entry where p is the start of the array of
void* pointers.

In that regards, changing the type of p from 'void **' (where you stride
by the size of a pointer when computing lp) to 'void *' (where you
stride by 1 under gcc when computing lp) is WRONG.

> I prefer void **p since that matches lp's and l1_map's type.

Not just prefer, but require.

> 
> It's true that since we're dealing with void * the compiler won't
> complain either way.

It's a shame that void* relaxes typing so much, but this is one case
where we HAVE to use the right type.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH 2/2] translate-all: Use proper type

[Pranith Kumar]

Eric Blake writes:

> On 10/18/2016 01:34 PM, Emilio G. Cota wrote:
>> +++ b/translate-all.c
>> @@ -405,23 +405,23 @@ static void page_init(void)
>>  static PageDesc *page_find_alloc(tb_page_addr_t index, int alloc)
>>  {
>>      PageDesc *pd;
>>      void **lp;
>>      int i;
>> 
>>      /* Level 1.  Always allocated.  */
>>      lp = l1_map + ((index >> V_L1_SHIFT) & (V_L1_SIZE - 1));
>> 
>>      /* Level 2..N-1.  */
>>      for (i = V_L1_SHIFT / V_L2_BITS - 1; i > 0; i--) {
>> -        void **p = atomic_rcu_read(lp);
>> +        void *p = atomic_rcu_read(lp);
>> 
>>          if (p == NULL) {
>>              if (!alloc) {
>>                  return NULL;
>>              }
>>              p = g_new0(void *, V_L2_SIZE);
>>              atomic_rcu_set(lp, p);
>>          }
>> 
>>          lp = p + ((index >> (i * V_L2_BITS)) & (V_L2_SIZE - 1));
>
> Pointer addition of 'void *' plus an offset is undefined (gcc, and
> presumably clang, have an extension that treats it the same as computing
> an offset to a 'char *'; but some compilers choke); this is because
> sizeof(void) is unknown, so you don't know what stride to make for each
> offset.  Or put another way, 'p + offset' is the same as
> '&((*p)[offset])', but (*p)[offset] is ill-defined when p is the opaque
> type void.
>
> Pointer addition of 'void **' plus an offset is well-defined, because
> sizeof(void*) is well-defined and therefore the stride (4 or 8) makes
> sense.  Or in array notation, computing '&((*p)[offset]) means we are
> skipping to the offset array entry where p is the start of the array of
> void* pointers.
>

Indeed. I missed that crucial detail. I would prefer explicitly casting to
'void **' for p, since that is not the type of what is being returned by
atomic_rcu_read().

The joys of void pointer arithmetic, TIL.

-- 
Pranith

Re: [Qemu-devel] [PATCH 1/2] docs/rcu: Distinguish rcu_dereference and atomic_rcu_read

[Paolo Bonzini]

----- Original Message -----
> From: "Pranith Kumar" <bobby.prani@gmail.com>
> To: "Paolo Bonzini" <pbonzini@redhat.com>, "Sergey Fedorov" <serge.fdrv@gmail.com>, "Cao jin"
> <caoj.fnst@cn.fujitsu.com>, "open list:All patches CC here" <qemu-devel@nongnu.org>
> Sent: Tuesday, October 18, 2016 4:56:19 PM
> Subject: [PATCH 1/2] docs/rcu: Distinguish rcu_dereference and atomic_rcu_read
> 
> Signed-off-by: Pranith Kumar <bobby.prani@gmail.com>
> ---
>  docs/rcu.txt | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/docs/rcu.txt b/docs/rcu.txt
> index c84e7f4..c177dcb 100644
> --- a/docs/rcu.txt
> +++ b/docs/rcu.txt
> @@ -197,7 +197,9 @@ DIFFERENCES WITH LINUX
>    critical section to become an updater.
>  
>  - atomic_rcu_read and atomic_rcu_set replace rcu_dereference and
> -  rcu_assign_pointer.  They take a _pointer_ to the variable being accessed.
> +  rcu_assign_pointer.  Note that although both atomic_rcu_read and
> +  rcu_dereference take a _pointer_ to the variable being accessed,
> +  atomic_rcu_read dereferences the pointer whereas rcu_dereference does not.

No, neither rcu_dereference nor rcu-assign_pointer take a pointer.  You use
them like rcu_dereference(p) versus QEMU's atomic_rcu_read(&p).

Paolo

>  - call_rcu is a macro that has an extra argument (the name of the first
>    field in the struct, which must be a struct rcu_head), and expects the
> --
> 2.10.1
> 
> 

Re: [Qemu-devel] [PATCH 2/2] translate-all: Use proper type

[Paolo Bonzini]

On 18/10/2016 16:56, Pranith Kumar wrote:
> gcc does not warn about the wrong type since it is a void pointer
> which can be cast to any type.
> 
> Signed-off-by: Pranith Kumar <bobby.prani@gmail.com>
> ---
>  translate-all.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/translate-all.c b/translate-all.c
> index 8ca393c..c77470a 100644
> --- a/translate-all.c
> +++ b/translate-all.c
> @@ -412,7 +412,7 @@ static PageDesc *page_find_alloc(tb_page_addr_t index, int alloc)
>  
>      /* Level 2..N-1.  */
>      for (i = V_L1_SHIFT / V_L2_BITS - 1; i > 0; i--) {
> -        void **p = atomic_rcu_read(lp);
> +        void *p = atomic_rcu_read(lp);

Wrong; you can see below that p is initialized with

	p = g_new0(void *, V_L2_SIZE);

so it must be a pointer to "void *".  You are introducing exactly the
bug that is mentioned in the commit message, and it would have screwed
up this statement:

        lp = p + ((index >> (i * V_L2_BITS)) & (V_L2_SIZE - 1));

because it would then omit the multiplication of the RHS by sizeof(void *).

How did you test the patch?  Coverity would have caught this, but please
be more careful.

Thanks,

Paolo

>          if (p == NULL) {
>              if (!alloc) {
>