RE: [RFC 1/3] /dev/low_mem_notify

[leonid.moiseichuk]

Hi,

Just couple of observations, which maybe wrong below

> -----Original Message-----
> From: Pekka Enberg [mailto:penberg@xxxxxxxxx] On Behalf Of ext Pekka
> Enberg
> Sent: 17 January, 2012 20:51
....

> +struct vmnotify_config {
> +	/*
> +	 * Size of the struct for ABI extensibility.
> +	 */
> +	__u32		   size;
> +
> +	/*
> +	 * Notification type bitmask
> +	 */
> +	__u64			type;
> +
> +	/*
> +	 * Free memory threshold in percentages [1..99]
> +	 */
> +	__u32			free_threshold;

Would be possible to not use percents for thesholds? Accounting in pages even not so difficult to user-space.
Also, looking on vmnotify_match I understand that events propagated to user-space only in case threshold trigger change state from 0 to 1 but not back, 1-> 0 is very useful event as well.

Would be possible to use for threshold pointed value(s) e.g. according to enum zone_state_item, because kinds of memory to track could be different?
E.g. to tracking paging activity NR_ACTIVE_ANON and NR_ACTIVE_FILE could be interesting, not only free.

> +
> +	/*
> +	 * Sample period in nanoseconds
> +	 */
> +	__u64			sample_period_ns;
> +};
> +
....
> +struct vmnotify_event {
> +	/* Size of the struct for ABI extensibility. */
> +	__u32			size;
> +
> +	__u64			nr_avail_pages;
> +
> +	__u64			nr_swap_pages;
> +
> +	__u64			nr_free_pages;
> +};

Two fields here most likely session-constant, (nr_avail_pages and nr_swap_pages), seems not much sense to report them in every event.
If we have memory/swap hotplug user-space can use sysinfo() call.

> +static void vmnotify_sample(struct vmnotify_watch *watch) {
...
> +	si_meminfo(&si);
> +	event.nr_avail_pages	= si.totalram;
> +
> +#ifdef CONFIG_SWAP
> +	si_swapinfo(&si);
> +	event.nr_swap_pages	= si.totalswap;
> +#endif
> +

Why not to use global_page_state() directly? si_meminfo() and especial si_swapinfo are quite expensive call.

> +static void vmnotify_start_timer(struct vmnotify_watch *watch) {
> +	u64 sample_period = watch->config.sample_period_ns;
> +
> +	hrtimer_init(&watch->timer, CLOCK_MONOTONIC,
> HRTIMER_MODE_REL);
> +	watch->timer.function = vmnotify_timer_fn;
> +
> +	hrtimer_start(&watch->timer, ns_to_ktime(sample_period),
> +HRTIMER_MODE_REL_PINNED); }

Do I understand correct you allocate timer for every user-space client and propagate events every pointed interval?
What will happened with system if we have a timer but need to turn CPU off? The timer must not be a reason to wakeup if user-space is sleeping.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/