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