Re: [PATCH 3/4] samples/cgroup: arm the inotify watch before reading the baseline

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] samples/cgroup: events before read_memcg_events() are absorbed into baseline
--

commit e11bb86059068b47a263deb2a44878c61af9a6da
Author: Song Hu <husong@kylinos.cn>

samples/cgroup: arm the inotify watch before reading the baseline

This commit moves the initial read_memcg_events() call in
initialize_memcg_events() to occur after inotify_add_watch() so that
events firing during initialization are not missed before the watch is
created.

> diff --git a/samples/cgroup/memcg_event_listener.c b/samples/cgroup/memcg_event_listener.c
> index 996bf6fb117c4..c23af43bf6ccd 100644
> --- a/samples/cgroup/memcg_event_listener.c
> +++ b/samples/cgroup/memcg_event_listener.c
> @@ -293,12 +293,6 @@ static int initialize_memcg_events(struct memcg_events *events,
>  		return ret;
>  	}
>  
> -	ret = read_memcg_events(events, /* show_diff = */false);
> -	if (ret) {
> -		warnx("Failed to read initial memcg events state (%d)", ret);
> -		return ret;
> -	}
> -
>  	events->inotify_fd = inotify_init();
>  	if (events->inotify_fd < 0) {
>  		warn("Failed to setup new inotify device");
> @@ -313,6 +307,17 @@ static int initialize_memcg_events(struct memcg_events *events,
>  		return -EIO;
>  	}
>  
> +	/*
> +	 * Read the initial state only after the watch is armed, so no
> +	 * event can slip through the window between the baseline read and
> +	 * the watch creation.
> +	 */
> +	ret = read_memcg_events(events, /* show_diff = */false);
> +	if (ret) {
> +		warnx("Failed to read initial memcg events state (%d)", ret);
> +		return ret;
> +	}

[Severity: Medium]
If a memcg event fires after inotify_add_watch() arms the watch in
initialize_memcg_events(), but before read_memcg_events(events, false)
reads the initial counters, won't that event be absorbed into the baseline
without its diff ever being reported?

When the event fires in that window, the kernel increments the cumulative
counter in memory.events and queues an IN_MODIFY notification on
events->inotify_fd. Then initialize_memcg_events() calls
read_memcg_events(events, false):

samples/cgroup/memcg_event_listener.c:read_memcg_events() {
    ...
	for (i = 0; i < ARRAY_SIZE(map); ++i) {
		long diff;

		if (*map[i].new > *map[i].old) {
			diff = *map[i].new - *map[i].old;

			if (show_diff)
				printf("*** %ld MEMCG %s event%s, "
				       "change counter %ld => %ld\n",
				       diff, map[i].name,
				       (diff == 1) ? "" : "s",
				       *map[i].old, *map[i].new);

			*map[i].old += diff;
			any_new_events = true;
		}
	}

	if (show_diff && !any_new_events)
		printf("*** No new untracked memcg events available\n");
    ...
}

Because show_diff is false, *map[i].old is updated to the new counter
value without printing the diff. When monitor_events() subsequently enters
poll() -> process_memcg_events() -> read_memcg_events(events, true) to
handle the queued IN_MODIFY notification, *map[i].new > *map[i].old is now
false, so no diff is reported and it instead prints:

  *** No new untracked memcg events available

Also, the commit message states that in the original order (reading the
baseline first and arming the watch afterwards), an event firing in
between "is already included in the baseline (no diff will ever be
reported) and no watch existed to queue a notification". Isn't that
description inverted? In the original order, an event firing after the
baseline read was not included in the baseline (it only lacked an inotify
notification until a subsequent event arrived), whereas with the new order
an event in the window is included in the baseline while queueing a
notification whose diff is already consumed.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261102122528.1498193-1-husong@kylinos.cn?part=3