Re: [PATCH v5 1/2] mm, memcg: cg2 memory{.swap,}.peak write handlers

["Michal Koutný" <mkoutny@suse.com>]

[-- Attachment #1: Type: text/plain, Size: 2000 bytes --]

Hello David.

On Wed, Jul 24, 2024 at 12:19:41PM GMT, David Finkel <davidf@vimeo.com> wrote:
> Writing a specific string to the memory.peak and memory.swap.peak
> pseudo-files reset the high watermark to the current usage for
> subsequent reads through that same fd.

This is elegant and nice work! (Caught my attention, so a few nits below.)

> --- a/include/linux/cgroup-defs.h
> +++ b/include/linux/cgroup-defs.h
> @@ -775,6 +775,11 @@ struct cgroup_subsys {
>  
>  extern struct percpu_rw_semaphore cgroup_threadgroup_rwsem;
>  
> +struct cgroup_of_peak {
> +	long			value;

Wouldn't this better be unsigned like watermarks themselves?

> +	struct list_head	list;
> +};


> --- a/include/linux/page_counter.h
> +++ b/include/linux/page_counter.h
> @@ -26,6 +26,7 @@ struct page_counter {
>  	atomic_long_t children_low_usage;
>  
>  	unsigned long watermark;
> +	unsigned long local_watermark;

At first, I struggled understading what the locality is (when the local
value is actually in of_peak), IIUC, it's more about temporal position.

I'd suggest a comment (if not a name) like:
        /* latest reset watermark */
> +	unsigned long local_watermark;


> +
> +	/* User wants global or local peak? */
> +	if (fd_peak == -1UL)

Here you use typed -1UL but not in other places. (Maybe define an
explicit macro value ((unsigned long)-1)?)

> +static ssize_t peak_write(struct kernfs_open_file *of, char *buf, size_t nbytes,
> +			  loff_t off, struct page_counter *pc,
> +			  struct list_head *watchers)
> +{
...
> +	list_for_each_entry(peer_ctx, watchers, list)
> +		if (usage > peer_ctx->value)
> +			peer_ctx->value = usage;

The READ_ONCE() in peak_show() suggests it could be WRITE_ONCE() here.

> +
> +	/* initial write, register watcher */
> +	if (ofp->value == -1)
> +		list_add(&ofp->list, watchers);
> +
> +	ofp->value = usage;

Move the registration before iteration and drop the extra assignment?

Thanks,
Michal

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]