Re: [PATCH 15/23] ivtv: Replace memcpy with struct assignment

Andy Walls
2012-10-23T22:08:48Z

On Tue, 2012-10-23 at 16:57 -0300, Ezequiel Garcia wrote:
> This kind of memcpy() is error-prone. Its replacement with a struct
> assignment is prefered because it's type-safe and much easier to read.

This one is a code maintenance win. :)

See my comments at the end for the difference in assembled code on an
AMD x86_64 CPU using
$ gcc --version
gcc (GCC) 4.6.3 20120306 (Red Hat 4.6.3-2)

> Found by coccinelle. Hand patched and reviewed.
> Tested by compilation only.
>
> A simplified version of the semantic match that finds this problem is as
> follows: (http://coccinelle.lip6.fr/)
>
> // <smpl>
> @@
> identifier struct_name;
> struct struct_name to;
> struct struct_name from;
> expression E;
> @@
> -memcpy(&(to), &(from), E);
> +to = from;
> // </smpl>
>
> Cc: Andy Walls <awalls@md.metrocast.net>

Signed-off-by: Andy Walls <awalls@md.metrocast.net>

> Signed-off-by: Peter Senna Tschudin <peter.senna@gmail.com>
> Signed-off-by: Ezequiel Garcia <elezegarcia@gmail.com>
> ---
> drivers/media/pci/ivtv/ivtv-i2c.c | 12 ++++--------
> 1 files changed, 4 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/media/pci/ivtv/ivtv-i2c.c b/drivers/media/pci/ivtv/ivtv-i2c.c
> index d47f41a..27a8466 100644
> --- a/drivers/media/pci/ivtv/ivtv-i2c.c
> +++ b/drivers/media/pci/ivtv/ivtv-i2c.c
> @@ -719,13 +719,10 @@ int init_ivtv_i2c(struct ivtv *itv)
> return -ENODEV;
> }
> if (itv->options.newi2c > 0) {
> - memcpy(&itv->i2c_adap, &ivtv_i2c_adap_hw_template,
> - sizeof(struct i2c_adapter));
> + itv->i2c_adap = ivtv_i2c_adap_hw_template;
> } else {
> - memcpy(&itv->i2c_adap, &ivtv_i2c_adap_template,
> - sizeof(struct i2c_adapter));
> - memcpy(&itv->i2c_algo, &ivtv_i2c_algo_template,
> - sizeof(struct i2c_algo_bit_data));
> + itv->i2c_adap = ivtv_i2c_adap_template;
> + itv->i2c_algo = ivtv_i2c_algo_template;
> }
> itv->i2c_algo.udelay = itv->options.i2c_clock_period / 2;
> itv->i2c_algo.data = itv;
> @@ -735,8 +732,7 @@ int init_ivtv_i2c(struct ivtv *itv)
> itv->instance);
> i2c_set_adapdata(&itv->i2c_adap, &itv->v4l2_dev);
>
> - memcpy(&itv->i2c_client, &ivtv_i2c_client_template,
> - sizeof(struct i2c_client));
> + itv->i2c_client = ivtv_i2c_client_template;
> itv->i2c_client.adapter = &itv->i2c_adap;
> itv->i2c_adap.dev.parent = &itv->pdev->dev;
>

I looked at the generated assembly with only this last change
implemented:

$ objdump -h -r -d -l -s orig-ivtv-i2c.o.sav | less
[...]
07e0 00000000 69767476 20696e74 65726e61 ....ivtv interna
07f0 6c000000 00000000 00000000 00000000 l...............
0800 00000000 00000000 00000000 00000000 ................
0810 00000000 00000000 00000000 00000000 ................
0820 00000000 00000000 00000000 00000000 ................
0830 00000000 00000000 00000000 00000000 ................
[...]
init_ivtv_i2c():
/home/andy/cx18dev/git/media_tree/drivers/media/video/ivtv/ivtv-i2c.c:738
13bb: 48 c7 c6 00 00 00 00 mov $0x0,%rsi
13be: R_X86_64_32S .rodata+0x7e0
13c2: 48 8d bb 30 04 01 00 lea 0x10430(%rbx),%rdi
13c9: b9 5a 00 00 00 mov $0x5a,%ecx
13ce: f3 48 a5 rep movsq %ds:(%rsi),%es:(%rdi)

$ objdump -h -r -d -l -s orig-ivtv-i2c.o.sav | less
[...]
07e0 00000000 69767476 20696e74 65726e61 ....ivtv interna
07f0 6c000000 00000000 00000000 00000000 l...............
0800 00000000 00000000 00000000 00000000 ................
0810 00000000 00000000 00000000 00000000 ................
0820 00000000 00000000 00000000 00000000 ................
0830 00000000 00000000 00000000 00000000 ................
[...]
init_ivtv_i2c():
/home/andy/cx18dev/git/media_tree/drivers/media/video/ivtv/ivtv-i2c.c:738
13bb: 48 8d bb 30 04 01 00 lea 0x10430(%rbx),%rdi
13c2: 48 c7 c6 00 00 00 00 mov $0x0,%rsi
13c5: R_X86_64_32S .rodata+0x7e0
13c9: b9 5a 00 00 00 mov $0x5a,%ecx
13ce: f3 48 a5 rep movsq %ds:(%rsi),%es:(%rdi)

The generated code is reordered, but essentially identical. So I guess
in this instance, the preprocessor defines resolved such that an x86-64
optimized memcpy() function was not used from the linux kernel source.

Since all of these memcpy()'s are only called once for each board at
board initialization, performance here really doesn't matter here
anyway. (Unless one is insanely trying to shave microseconds off boot
time :P )

With other memcpy()/assignement_operator replacement patches, you may
wish to keep performance in mind, if you are patching a frequently
called function.

Regards,
Andy