Re: [PATCH 0/3] cgroup/rdma: add rdma.peak and rdma.events[.local]

[Tejun Heo <tj@kernel.org>]

Hello,

The list below is from an AI-assisted review with some input from me.

* Patches 2 and 3 don't extend the rpool-free condition in
  uncharge_cg_locked() and rdmacg_resource_set_max() to the new event
  counters, so a "set limit -> hit limit -> uncharge to 0 -> write
  'max max'" sequence frees the rpool and zeros the counts.

* rdmacg_event_locked() creates rpools in ancestors of over_cg via
  get_cg_rpool_locked() just to host event counters. Those rpools have
  usage_sum==0, num_max_cnt==max, peak==0, so the next real uncharge
  through any such ancestor frees them.

* Patch 3 says failcnt covers "this cgroup (or its descendants)" but
  the code only increments the directly-requesting cgroup. Either the
  description or the propagation is wrong.

* rdma.events / rdma.events.local print "mlx4_0 hca_handle.max 5
  hca_object.max 0 " (trailing space). That doesn't match any of the
  formats in Documentation/admin-guide/cgroup-v2.rst. rdma.current and
  rdma.max are nested-keyed; the new files should be too:
  "mlx4_0 hca_handle.max=5 hca_object.max=0".

* Please document rdma.peak / rdma.events / rdma.events.local in
  Documentation/admin-guide/cgroup-v2.rst.

* "failcnt" is cgroup-v1 vocabulary; pids.events.local uses
  "fork_fail" for the same role.

* Event counters are atomic64_t but all updates are under
  rdmacg_mutex. Plain u64 with READ_ONCE on the read side would do.

* Patch 1 reflows an unrelated comment ("No user of the rpool ...");
  please drop the churn.

Thanks.

--
tejun