On Tue, Jul 23, 2024 at 09:55:19PM -0400, Waiman Long wrote:
Could you use the "-v <n>" option of git-format-patch to add a version number to the patch title? Without that, it can be confusing as to whether the patch is new or a resend of the previous one.
+1
@@ -775,6 +775,11 @@ struct cgroup_subsys { extern struct percpu_rw_semaphore cgroup_threadgroup_rwsem; +struct cgroup_of_peak {
- long value;
- struct list_head list;
+};
The name "cgroup_of_peak" is kind of confusing. Maybe local_peak?
It's the peak associated with an 'of' (which is a known concept in cgroup code), and it pairs up nicely with of_peak(). I'd prefer keeping that over local_peak.
@@ -26,6 +26,7 @@ struct page_counter { atomic_long_t children_low_usage; unsigned long watermark;
- unsigned long local_watermark; /* track min of fd-local resets */
track "min"? I thought it is used to track local maximum after a reset.
Yeah, the comment doesn't sound quite right.
However, I think we'd be hard-pressed to explain correctly and comprehensively what this thing does in <40 characters.
I'd just remove the comment tbh.
@@ -78,7 +79,10 @@ int page_counter_memparse(const char *buf, const char *max, static inline void page_counter_reset_watermark(struct page_counter *counter) {
- counter->watermark = page_counter_read(counter);
- unsigned long usage = page_counter_read(counter);
- counter->watermark = usage;
- counter->local_watermark = usage; }
Could you set the local_watermark first before setting watermark? There is a very small time window that the invariant "local_watermark <= watermark" is not true.
Does it matter? Only cgroup1 supports global resets; only cgroup2 supports local peaks watching. This doesn't add anything to the race that already exists between reset and global watermark update on cg1.
@@ -3950,12 +3955,90 @@ static u64 memory_current_read(struct cgroup_subsys_state *css, return (u64)page_counter_read(&memcg->memory) * PAGE_SIZE; } -static u64 memory_peak_read(struct cgroup_subsys_state *css,
struct cftype *cft)+static int peak_show(struct seq_file *sf, void *v, struct page_counter *pc) {
- struct mem_cgroup *memcg = mem_cgroup_from_css(css);
- struct cgroup_of_peak *ofp = of_peak(sf->private);
- s64 fd_peak = ofp->value, peak;
- /* User wants global or local peak? */
- if (fd_peak == -1)
peak = pc->watermark;- else
peak = max(fd_peak, (s64)pc->local_watermark);Should you save the local_watermark value into ofp->value if local_watermark is bigger? This will ensure that each successive read of the fd is monotonically increasing. Otherwise the value may go up or down if there are multiple resets in between.
The reset saves local_watermark into ofp->value if it's bigger..?
I do see another problem, though. The compiler might issue multiple reads to ofp->value in arbitrary order. We could print max(-1, ...) which is nonsense. Saving ofp->value into a local variable is the right idea, but the compiler might still issue two reads anyway. It needs a READ_ONCE() to force a single read.
I'd use unsigned long for fd_peak. This way the "specialness" is on the -1UL comparison. The max() must be between two positive numbers, so the (s64) there is confusing.