Is this code right in zram?
before I go stomping all over other peoples' work and sending idiotic patches, I think I'll ask. Since I have no clue what I'm doing.
in drivers/staging/zram.c out of 3.4 (I just grabbed the source hours ago), I see this on lines 810-822:
/* Allocate the device array and initialize each one */
pr_info("Creating %u devices ...\n", num_devices);
zram_devices = kzalloc(num_devices * sizeof(struct zram),
GFP_KERNEL);
if (!zram_devices) {
ret = -ENOMEM;
goto unregister;
}
for (dev_id = 0; dev_id < num_devices; dev_id++) {
ret = create_device(&zram_devices[dev_id], dev_id);
if (ret)
goto free_devices;
}Curiosity got me to here:
http://lwn.net/Articles/147014/
So assuming this, what I see here is:
- kmalloc(num_devices * sizeof(struct zram), GFP_KERNEL);
- memset() that to 0
- immediately fill in this RAM without reading it
I'm wondering what the immediate need is to fill the area with zeros? Also curious as to whether the kzalloc() thing should better be kcalloc(num_devices, sizeof(struct zram), GFP_KERNEL) as a matter of convention.
Quoted 5 lines · John Moser
before I go stomping all over other peoples' work and sending idiotic patches, I think I'll ask. Since I have no clue what I'm doing.
in
drivers/staging/zram.cout of 3.4
There's no such file in 3.4 - I get
tree v3.4:drivers/staging/zram/
KconfigMakefile
zram.txtzram_drv.czram_drv.hzram_sysfs.c
I'm guessing you mean zram_drv.c - there's code like that below?
Quoted 22 lines
(I just grabbed the source
hours ago), I see this on lines 810-822:/* Allocate the device array and initialize each one */ pr_info("Creating %u devices ...\n", num_devices); zram_devices = kzalloc(num_devices * sizeof(struct zram), GFP_KERNEL); if (!zram_devices) { ret = -ENOMEM; goto unregister; } for (dev_id = 0; dev_id < num_devices; dev_id++) { ret = create_device(&zram_devices[dev_id], dev_id); if (ret) goto free_devices; }Curiosity got me to here:
Quoted 8 lines
So assuming this, what I see here is:
-
kmalloc(num_devices * sizeof(struct zram), GFP_KERNEL);
-memset()that to 0
- immediately fill in this RAM without reading itI'm wondering what the immediate need is to fill the area with zeros?
You don't want to use unitialized memory in those zram_devices thingies, i.e. touch something in there which you haven't initialized before thus the convention to zero out the whole struct.
Right, create_device() does that init later but what do you do if create_device changes and forgets one variable, for example? There's your bug (and a very subtle one, for that matter).
That's why you can't risk it and have to init the memory to a known good value just in case.
Quoted 3 lines
Also curious as to whether the
kzalloc()thing should better bekcalloc(num_devices, sizeof(struct zram), GFP_KERNEL)as a matter of convention.
That makes sense, let's ask.
Signature · 2 lines
Regards/Gruss,
Boris.Quoted 5 lines · John Moser
before I go stomping all over other peoples' work and sending idiotic patches, I think I'll ask. Since I have no clue what I'm doing.
in
drivers/staging/zram.cout of 3.4 (I just grabbed the source hours ago), I see this on lines 810-822:
I assume that's drivers/staging/zram_drv.c.
Quoted 20 lines
/* Allocate the device array and initialize each one */ pr_info("Creating %u devices ...\n", num_devices); zram_devices = kzalloc(num_devices * sizeof(struct zram), GFP_KERNEL); if (!zram_devices) { ret = -ENOMEM; goto unregister; } for (dev_id = 0; dev_id < num_devices; dev_id++) { ret = create_device(&zram_devices[dev_id], dev_id); if (ret) goto free_devices; }Curiosity got me to here:
Quoted 7 lines
So assuming this, what I see here is:
-
kmalloc(num_devices * sizeof(struct zram), GFP_KERNEL);
-memset()that to 0
- immediately fill in this RAM without reading itI'm wondering what the immediate need is to fill the area with zeros?
It's to avoid to have undefined values between the time the device is created and initialized. In that case, it may be superfluous since the locking mechanism should avoid to access an uninitialized device.
Quoted 3 lines
Also curious as to whether the
kzalloc()thing should better bekcalloc(num_devices, sizeof(struct zram), GFP_KERNEL)as a matter of convention.
It probably doesn't matter much. Apparently kcalloc() never got a lot of success and kzalloc(sizeof(foo) * num_foos) type of allocation are more popular.
Jerome
Quoted 5 lines
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Quoted 1 line · Jerome Marchand
It probably doesn't matter much.
Likely true.
Quoted 3 lines
Apparently
kcalloc()never got a lot
of success andkzalloc(sizeof(foo) * num_foos)type of allocation are more popular.
Likely false, especially in new code.
If you did greps, perhaps you are including the
k.alloc(sizeof(*foo), GFP)
forms in your uses multiply counts.
Quoted 9 lines · Joe Perches
Quoted 1 line · Jerome Marchand
It probably doesn't matter much.
Likely true.
Quoted 3 lines
Apparently
kcalloc()never got a lot
of success andkzalloc(sizeof(foo) * num_foos)type of allocation are more popular.
Quoted 5 lines
Likely false, especially in new code.
If you did greps, perhaps you are including the
k.alloc(sizeof(*foo), GFP)forms in your uses multiply counts.
No. Actually I greped it and look quickly at which proportion of those kzalloc() looked like the above. I have to admit that my sample was small and certainly not evenly distributed around the source tree.
Jerome
Quoted 7 lines
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/