| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20120104195521.GA19181@suse.de>
Date: Wed, 4 Jan 2012 11:55:21 -0800
From: Greg KH <gregkh@...e.de>
To: Leonid Moiseichuk <leonid.moiseichuk@...ia.com>
Cc: linux-mm@...ck.org, linux-kernel@...r.kernel.org,
cesarb@...arb.net, kamezawa.hiroyu@...fujitsu.com,
emunson@...bm.net, penberg@...nel.org, aarcange@...hat.com,
riel@...hat.com, mel@....ul.ie, rientjes@...gle.com,
dima@...roid.com, rebecca@...roid.com, san@...gle.com,
akpm@...ux-foundation.org, vesa.jaaskelainen@...ia.com
Subject: Re: [PATCH 3.2.0-rc1 3/3] Used Memory Meter pseudo-device module
On Wed, Jan 04, 2012 at 07:21:56PM +0200, Leonid Moiseichuk wrote:
> The Used Memory Meter (UMM) device tracks level of memory utilization
> and notifies subscribed processes when consumption crossed specified
> threshold up or down. It could be used on embedded devices to
> implementation of performance-cheap memory reacting by using
> e.g. libmemnotify or similar user-space component.
>
> Signed-off-by: Leonid Moiseichuk <leonid.moiseichuk@...ia.com>
Note, I don't agree that this code is the correct thing to be doing
here, you'll have to get the buy-in from the mm developers on that, but
I do have some comments on the implementation:
> --- /dev/null
> +++ b/drivers/misc/umm.c
> @@ -0,0 +1,452 @@
> +/*
> + * umm.c - system-wide Used Memory Meter pseudo-device implementation
> + *
> + * Copyright (C) 2011 Nokia Corporation.
> + * Leonid Moiseichuk
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * This program is distributed "as is" WITHOUT ANY WARRANTY of any
> + * kind, whether express or implied; without even the implied warranty
> + * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + */
> +
> +#include <linux/types.h>
> +#include <linux/module.h>
> +#include <linux/device.h>
> +#include <linux/kernel.h>
> +#include <linux/atomic.h>
> +#include <linux/jiffies.h>
> +#include <linux/mm.h>
> +#include <linux/slab.h>
> +#include <linux/poll.h>
> +#include <linux/highmem.h>
> +#include <linux/swap.h>
> +#include <linux/list.h>
> +#include <linux/wait.h>
> +#include <linux/spinlock.h>
> +#include <linux/spinlock_types.h>
> +
> +#include <linux/umm.h>
Why do you need a header file at all?
> +/* subscriber information to be notified when level changed */
> +struct observer {
> + /* list data to check from notify_memory_usage and wakeup user-space */
> + struct list_head list;
> + /* related file structure for open/close/read/write and poll */
> + struct file *file;
> + /* threshold [pages] when we should trigger notification */
> + unsigned long threshold;
> + /* did we crossed theshold on last validation? */
> + bool active;
> + /* flag about new notification is required */
> + bool updated;
> +};
> +
> +
> +
> +MODULE_AUTHOR("Leonid Moiseichuk (leonid.moiseichuk@...ia.com)");
> +MODULE_DESCRIPTION("System used memory meter pseudo-device");
> +MODULE_LICENSE("GPL v2");
> +MODULE_VERSION("0.0.2");
> +
> +static int debug __read_mostly;
> +module_param(debug, bool, 0);
> +MODULE_PARM_DESC(debug, "More info about module parameters and operations");
> +
> +static int probe __read_mostly;
> +module_param(probe, bool, 0);
> +MODULE_PARM_DESC(probe, "Probe measurement overhead during loading");
> +
> +static char device_name[64] __read_mostly = UMM_DEVICE_NAME;
> +module_param_string(device_name, device_name, sizeof(device_name), 0);
> +MODULE_PARM_DESC(device_name, "Device name in /dev if need different");
This is pointless, right?
> +static struct device *umm_device __read_mostly;
Enough with the __read_mostly markings, they really aren't needed for
every single variable, right? Especially for trivial stuff like this
one, and all of the module parameters.
> +static struct class *umm_class __read_mostly;
Just use a misc device, as you are only creating/needing one character
device, right? That will make your init and destroy code a lot cleaner
and smaller.
> + pr_info("UMM: Used Memory Meter loading to support /dev/%s\n",
> + device_name);
Not needed.
> +
> + umm_major = register_chrdev(0, device_name, &umm_fops);
> + if (umm_major < 0) {
> + pr_err("UMM: unable to get major number for device %s\n",
> + device_name);
> + error = -EBUSY;
> + goto register_failed;
> + }
> +
> + umm_class = class_create(THIS_MODULE, device_name);
> + if (IS_ERR(umm_class)) {
> + error = PTR_ERR(umm_class);
> + pr_err("UMM: unable to create class for device %s - %d\n",
> + device_name, error);
> + goto class_failed;
> + }
> +
> + umm_device = device_create(
> + umm_class, NULL,
> + MKDEV(umm_major, 0),
> + NULL, device_name);
> + if (IS_ERR(umm_device)) {
> + error = PTR_ERR(umm_device);
> + pr_err("UMM: unable to create device %s - %d\n",
> + device_name, error);
> + goto device_failed;
> + }
> +
> + update_period_jiffies = msecs_to_jiffies(update_period);
> + if (!update_period_jiffies)
> + update_period_jiffies = msecs_to_jiffies(UMM_UPDATE_PERIOD);
> +
> + /* query amount of available ram and swap, mem_unit is PAGE_SIZE */
> + si_meminfo(&si);
> +#ifdef CONFIG_SWAP
> + si_swapinfo(&si);
> + available_pages = si.totalram + si.totalswap;
> + available_swap_pages = si.totalswap;
> +#else
> + available_pages = si.totalram;
> +#endif
> + /* if autodetect then set granularity to ~1.4% from available memory */
> + if (update_space)
> + update_space_pages = update_space >> (PAGE_SHIFT - 10);
> + else
> + update_space_pages = available_pages >> 6;
> + if (!update_space_pages)
> + update_space_pages = UMM_UPDATE_SPACE >> (PAGE_SHIFT - 10);
> +
> + update_memory_usage();
> + old_mm_hook = set_mm_alloc_free_hook(mm_alloc_free_hook);
> +
> + if (debug) {
> + pr_info("UMM: /dev/%s got major %d\n", device_name, umm_major);
> + pr_info("UMM: update period set to %u ms or %lu jiffies\n",
> + update_period, update_period_jiffies);
> + pr_info("UMM: update space set to %u kb or %u pages\n",
> + update_space, update_space_pages);
> + pr_info("UMM: old mm alloc/free hook is 0x%p\n", old_mm_hook);
> + pr_info("UMM: now hook set to 0x%p\n", mm_alloc_free_hook);
> +#ifdef CONFIG_SWAP
> + pr_info("UMM: %lu available pages found (only ram)\n",
> + available_pages);
> +#else
> + pr_info("UMM: %lu available pages found (%lu ram + %lu swap)\n",
> + available_pages, si.totalram, si.totalswap);
> +#endif
> + pr_info("UMM: %lu used pages, utilization %lu percents\n",
> + atomic_long_read(&last_used_pages),
> + (100 * atomic_long_read(&last_used_pages)) /
> + available_pages);
> + pr_info("UMM: overhead per client connection is %lu bytes\n",
> + sizeof(struct observer));
Please use the dev_dbg() macro instead, that removes the debug flag
here, and properly identifies your code/device in the kernel log. Same
goes for other pr_* usages in the file, just use the appropriate dev_*
call instead.
thanks,
greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists