RE: Redundant memset in AIO read_events

RE: Redundant memset in AIO read_events

[Chen, Kenneth W]

> <newbie>
> There might be some architecture that requires 16 byte alignment...
> how about surrounding the memcpy with 
> if (sizeof(struct io_event) != 4 * sizeof(__u64)) ?
> </newbie>

The point I'm trying to make is that it is irrelevant with respect to
alignment, size, or padding.  memset and struct copying has the same
length and destination address.  Why bother with the memset?  It's the
same as writing a code like this:

blah = 0;
blah = foo;

- Ken

RE: Redundant memset in AIO read_events

[Chen, Kenneth W]

>> OK, here is another one.  In the top level read_events() function in
>> fs/aio.c, a struct io_event is instantiated on the stack (variable
ent).
>> It calls aio_read_evt() function which will fill the entire io_event
>> structure into variable ent.  What's the point of zeroing when copy
>> covers the same memory area?  Possible a debug code left around?
>
>Read the comment before that memset. The structure might contain some
>padding (bytes not belonging to any of its entries), these bytes are
>random and if you do not zero them, you copy random data into
userspace.
>
>Mikulas

Please have the courtesy of reading the relevant aio code in the entire
context.  The comment doesn't make any sense for the current state of
the code.  My guess is that it is left from stone age.

read_events(...) {
   struct io_event ent;
   memset(&ent, 0, sizeof(ent));
   while (...) {
      aio_read_evt(ctx, &ent);
   }
   ...
}

read_events calls aio_read_evt and passes pointer to ent as argument.

aio_read_evt(..., struct io_event *ent) {
  ...
  *ent = *evp;
  ...
}

Where on earth is the padding with respect to memset and struct copying?

If you are still not convinced and having doubts in understanding the C
code, then look at the assembly:

In read_events():
     e64:       31 c0                   xor    %eax,%eax
     ....
     e98:       b9 08 00 00 00          mov    $0x8,%ecx
     e9d:       f3 ab                   repz stos %eax,%es:(%edi)
<- 32 byte memset ->

In aio_read_evt()
     ddf:       8b 34 24                mov    (%esp,1),%esi
     de2:       b9 08 00 00 00          mov    $0x8,%ecx
     de7:       f3 a5                   repz movsl %ds:(%esi),%es:(%edi)
<- 32 byte structure copying ->

Now tell me again where on earth it has anything to do with padding?  If
there is any padding going on, it is overridden by the source of the
structure copying, effectively nullify the attempt of memset.

- Ken

Re: Redundant memset in AIO read_events

[Dan Kegel]

Luck, Tony wrote:
> That is true, but here's the definition of the io_event strcuture:
> 
> struct io_event {
>         __u64           data;
>         __u64           obj;
>         __s64           res;
>         __s64           res2;
> };
> 
> In the words of the comment, C may be "fun", but I've
> having trouble envisioning an architecture where a structure
> that consists of four equal sized objects has some padding!

<newbie>
There might be some architecture that requires 16 byte alignment...
how about surrounding the memcpy with if (sizeof(struct io_event) != 4 * sizeof(__u64)) ?
</newbie>

-- 
Dan Kegel
http://www.kegel.com
http://counter.li.org/cgi-bin/runscript/display-person.cgi?user=78045

RE: Redundant memset in AIO read_events

[Luck, Tony]

> > OK, here is another one.  In the top level read_events() function in
> > fs/aio.c, a struct io_event is instantiated on the stack 
> (variable ent).
> > It calls aio_read_evt() function which will fill the entire io_event
> > structure into variable ent.  What's the point of zeroing when copy
> > covers the same memory area?  Possible a debug code left around?
> 
> Read the comment before that memset. The structure might contain some
> padding (bytes not belonging to any of its entries), these bytes are
> random and if you do not zero them, you copy random data into 
> userspace.

That is true, but here's the definition of the io_event strcuture:

struct io_event {
        __u64           data;
        __u64           obj;
        __s64           res;
        __s64           res2;
};

In the words of the comment, C may be "fun", but I've
having trouble envisioning an architecture where a structure
that consists of four equal sized objects has some padding!

Don't we usually call code that defends against impossible
problems "bloat"?

-Tony

Re: Redundant memset in AIO read_events

[Mikulas Patocka]

> OK, here is another one.  In the top level read_events() function in
> fs/aio.c, a struct io_event is instantiated on the stack (variable ent).
> It calls aio_read_evt() function which will fill the entire io_event
> structure into variable ent.  What's the point of zeroing when copy
> covers the same memory area?  Possible a debug code left around?

Read the comment before that memset. The structure might contain some
padding (bytes not belonging to any of its entries), these bytes are
random and if you do not zero them, you copy random data into userspace.

Mikulas

Redundant memset in AIO read_events

[Chen, Kenneth W]

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

OK, here is another one.  In the top level read_events() function in
fs/aio.c, a struct io_event is instantiated on the stack (variable ent).
It calls aio_read_evt() function which will fill the entire io_event
structure into variable ent.  What's the point of zeroing when copy
covers the same memory area?  Possible a debug code left around?

- Ken
 <<aio.ent.patch>> 

[-- Attachment #2: aio.ent.patch --]
[-- Type: application/octet-stream, Size: 418 bytes --]

diff -Nur linux-2.5.74/fs/aio.c linux-2.5.74.aio/fs/aio.c
--- linux-2.5.74/fs/aio.c	Sun Jun 22 11:32:43 2003
+++ linux-2.5.74.aio/fs/aio.c	Tue Jul  8 16:11:50 2003
@@ -805,10 +805,6 @@
 	struct io_event		ent;
 	struct timeout		to;
 
-	/* needed to zero any padding within an entry (there shouldn't be 
-	 * any, but C is fun!
-	 */
-	memset(&ent, 0, sizeof(ent));
 	ret = 0;
 
 	while (likely(i < nr)) {