Hello David.
On Wed, Jul 24, 2024 at 12:19:41PM GMT, David Finkel <davidf@xxxxxxxxx> 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:
signature.asc
Description: PGP signature
