mbox series

[0/1] x86/resctrl: fix task CLOSID update race

Message ID 20221103141641.3055981-1-peternewman@google.com
Headers
Series x86/resctrl: fix task CLOSID update race |

Message

Peter Newman Nov. 3, 2022, 2:16 p.m. UTC 
  Hi Reinette, Fenghua,

Below is my patch to address the IPI race we discussed in the container
move RFD thread[1].

The patch below uses the new task_call_func() interface to serialize
updating closid and rmid with any context switch of the task. AFAICT,
the implementation of this function acts like a mutex with context
switch, but I'm not certain whether it is intended to be one. If this is
not how task_call_func() is meant to be used, I will instead move the
code performing the update under sched/ where it can be done holding the
task_rq_lock() explicitly, as Reinette has suggested before[2].

From my own measurements, this change will double the time to complete a
mass-move operation, such as rmdir on an rdtgroup with a large task
list. But to the best of my knowedge, these large-scale reconfigurations
of the control groups are infrequent, and the baseline I'm measuring
against is racy anyways.

What's still unclear to me is, when processing a large task list, is
obtaining the pi/rq locks for thousands of tasks (all while read-locking
the tasklist_lock) better than just blindly notifying all CPUs? My guess
is that the situation where notifying all CPUs would be better is
uncommon for most users and probably more likely in Google's use case
than most others, as we have a use case for moving large container jobs
to a different MBA group.

Thanks!
-Peter

[1] https://lore.kernel.org/all/CALPaoCg2-9ARbK+MEgdvdcjJtSy_2H6YeRkLrT97zgy8Aro3Vg@mail.gmail.com/
[2] https://lore.kernel.org/lkml/d3c06fa3-83a4-7ade-6b08-3a7259aa6c4b@intel.com/

Peter Newman (1):
  x86/resctrl: serialize task CLOSID update with task_call_func()

 arch/x86/include/asm/resctrl.h         | 11 ++--
 arch/x86/kernel/cpu/resctrl/rdtgroup.c | 83 +++++++++++++++-----------
 2 files changed, 51 insertions(+), 43 deletions(-)

Comments

Reinette Chatre Nov. 8, 2022, 6:49 p.m. UTC | #1 
Hi Peter,

On 11/3/2022 7:16 AM, Peter Newman wrote:
> Below is my patch to address the IPI race we discussed in the container
> move RFD thread[1].

Thank you very much for taking this on.

> 
> The patch below uses the new task_call_func() interface to serialize
> updating closid and rmid with any context switch of the task. AFAICT,
> the implementation of this function acts like a mutex with context
> switch, but I'm not certain whether it is intended to be one. If this is
> not how task_call_func() is meant to be used, I will instead move the
> code performing the update under sched/ where it can be done holding the
> task_rq_lock() explicitly, as Reinette has suggested before[2].
> 
> From my own measurements, this change will double the time to complete a
> mass-move operation, such as rmdir on an rdtgroup with a large task
> list. But to the best of my knowedge, these large-scale reconfigurations
> of the control groups are infrequent, and the baseline I'm measuring
> against is racy anyways.
> 
> What's still unclear to me is, when processing a large task list, is
> obtaining the pi/rq locks for thousands of tasks (all while read-locking
> the tasklist_lock) better than just blindly notifying all CPUs? My guess
> is that the situation where notifying all CPUs would be better is
> uncommon for most users and probably more likely in Google's use case
> than most others, as we have a use case for moving large container jobs
> to a different MBA group.
> 

It was unclear to me also so I asked for advice and learned that, in
general, sending extra IPIs is not evil. I learned that there is precedent
for sending unnecessary IPIs, for example, in the TLB flushing code where
it is common to land in the TLB flush IPI hander and learn that the TLB
does not need to be flushed. Highlighting that the user initiated resctrl
flow in question is rare when compared to TLB flush.

From what I understand even going through the extra locking and resulting
delays to avoid unnecessary IPIs with usage of task_call_func() it is still
possible to send unnecessary IPIs because the information about where modified
tasks are running may be stale by the time the IPIs are sent. To me it seems that
the risk of stale information increases as the size of the moved task group
increases. The benefit of using task_call_func() when moving a group of
tasks is thus not clear to me.

I do not see it as an either/or though. I think that using task_call_func()
to serialize with context switching is a good idea when moving a single
task. Sending IPIs to all CPUs in this case seems overkill. On the other hand,
when moving a group of tasks I think that notifying all CPUs would be
simpler. The current code already ensures that it does not modify the
PQR register unnecessarily. I would really like to learn more about this
from the experts but at this point I am most comfortable with such a
solution and look forward to to learning from the experts when it is
presented to the x86 maintainers for inclusion.

Reinette
Peter Newman Nov. 10, 2022, 3:28 p.m. UTC | #2 
Hi Reinette,

On Tue, Nov 8, 2022 at 7:50 PM Reinette Chatre
<reinette.chatre@intel.com> wrote:
> I do not see it as an either/or though. I think that using task_call_func()
> to serialize with context switching is a good idea when moving a single
> task. Sending IPIs to all CPUs in this case seems overkill. On the other hand,
> when moving a group of tasks I think that notifying all CPUs would be
> simpler. The current code already ensures that it does not modify the
> PQR register unnecessarily. I would really like to learn more about this
> from the experts but at this point I am most comfortable with such a
> solution and look forward to to learning from the experts when it is
> presented to the x86 maintainers for inclusion.

Great, that should allow me to post my mon group rename patch
independent of this one. As it is written today, it depends on this
patch to reliably notify moved tasks' CPUs.

Thanks!
-Peter