| Message ID | 20220422195516.10769-2-jvgediya@linux.ibm.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | mm: demotion: Introduce new node state N_DEMOTION_TARGETS | expand |
> Subject: [PATCH v3 1/7] mm: demotion: Fix demotion targets sharing among sources
IMHO, this patch doesn't fix some bugs in the original code. Instead,
it just enhances the original code. For example, the subject could be,
mm: demotion: support to share demotion targets among sources
Best Regards,
Huang, Ying
[snip]
On Sun, Apr 24, 2022 at 11:25:50AM +0800, ying.huang@intel.com wrote: > > Subject: [PATCH v3 1/7] mm: demotion: Fix demotion targets sharing among sources > > IMHO, this patch doesn't fix some bugs in the original code. Instead, > it just enhances the original code. For example, the subject could be, I think it is fixing a bug, there is a comment in the code which mentions that 'used_targets will become unavailable in future passes. This limits some opportunities for multiple source nodes to share a destination'. As per this comment, it was intended to share a node as demotion targets with some limits but the code limits not some but all such opportunities as no common node can be demotion targets for multiple source node as per current code. > mm: demotion: support to share demotion targets among sources > > Best Regards, > Huang, Ying > > [snip] > > >
On Mon, 2022-04-25 at 15:02 +0530, Jagdish Gediya wrote: > On Sun, Apr 24, 2022 at 11:25:50AM +0800, ying.huang@intel.com wrote: > > > Subject: [PATCH v3 1/7] mm: demotion: Fix demotion targets sharing among sources > > > > IMHO, this patch doesn't fix some bugs in the original code. Instead, > > it just enhances the original code. For example, the subject could be, > > I think it is fixing a bug, there is a comment in the code which > mentions that 'used_targets will become unavailable in future passes. > This limits some opportunities for multiple source nodes to share a > destination'. As per this comment, it was intended to share a node as > demotion targets with some limits but the code limits not some but all > such opportunities as no common node can be demotion targets for > multiple source node as per current code. IMHO, the original code is just to keep as simple as possible to address the issue for the real machines at that time. That provides a base line for future improvement like you have done. If the original code wouldn't work well for the target machines, then we fixed a bug. But if that doen't work well for some new kind of machines, then we need to improve the code, add more feature, not to fix a bug. Best Regards, Huang, Ying > > mm: demotion: support to share demotion targets among sources > > > > Best Regards, > > Huang, Ying > > > > [snip] > > > > > >
diff --git a/mm/migrate.c b/mm/migrate.c index 6c31ee1e1c9b..8bbe1e478122 100644 --- a/mm/migrate.c +++ b/mm/migrate.c @@ -2355,7 +2355,7 @@ static void __set_migration_target_nodes(void) { nodemask_t next_pass = NODE_MASK_NONE; nodemask_t this_pass = NODE_MASK_NONE; - nodemask_t used_targets = NODE_MASK_NONE; + nodemask_t source_nodes = NODE_MASK_NONE; int node, best_distance; /* @@ -2373,20 +2373,23 @@ static void __set_migration_target_nodes(void) again: this_pass = next_pass; next_pass = NODE_MASK_NONE; + /* - * To avoid cycles in the migration "graph", ensure - * that migration sources are not future targets by - * setting them in 'used_targets'. Do this only - * once per pass so that multiple source nodes can - * share a target node. - * - * 'used_targets' will become unavailable in future - * passes. This limits some opportunities for - * multiple source nodes to share a destination. + * Accumulate source nodes to avoid the cycle in migration + * list. */ - nodes_or(used_targets, used_targets, this_pass); + nodes_or(source_nodes, source_nodes, this_pass); for_each_node_mask(node, this_pass) { + /* + * To avoid cycles in the migration "graph", ensure + * that migration sources are not future targets by + * setting them in 'used_targets'. Reset used_targets + * to source nodes for each node in this pass so that + * multiple source nodes can share a target node. + */ + nodemask_t used_targets = source_nodes; + best_distance = -1; /*
Sharing used_targets between multiple nodes in a single pass limits some of the opportunities for demotion target sharing. Don't share the used targets between multiple nodes in a single pass, instead accumulate all the used targets in source nodes shared by all pass, and reset 'used_targets' to source nodes while finding demotion targets for any new node. This results into some more opportunities to share demotion targets between multiple source nodes, e.g. with below NUMA topology, where node 0 & 1 are cpu + dram nodes, node 2 & 3 are equally slower memory only nodes, and node 4 is slowest memory only node, available: 5 nodes (0-4) node 0 cpus: 0 1 node 0 size: n MB node 0 free: n MB node 1 cpus: 2 3 node 1 size: n MB node 1 free: n MB node 2 cpus: node 2 size: n MB node 2 free: n MB node 3 cpus: node 3 size: n MB node 3 free: n MB node 4 cpus: node 4 size: n MB node 4 free: n MB node distances: node 0 1 2 3 4 0: 10 20 40 40 80 1: 20 10 40 40 80 2: 40 40 10 40 80 3: 40 40 40 10 80 4: 80 80 80 80 10 The existing implementation gives below demotion targets, node demotion_target 0 3, 2 1 4 2 X 3 X 4 X With this patch applied, below are the demotion targets, node demotion_target 0 3, 2 1 3, 2 2 4 3 4 4 X e.g. with below NUMA topology, where node 0, 1 & 2 are cpu + dram nodes and node 3 is slow memory node, available: 4 nodes (0-3) node 0 cpus: 0 1 node 0 size: n MB node 0 free: n MB node 1 cpus: 2 3 node 1 size: n MB node 1 free: n MB node 2 cpus: 4 5 node 2 size: n MB node 2 free: n MB node 3 cpus: node 3 size: n MB node 3 free: n MB node distances: node 0 1 2 3 0: 10 20 20 40 1: 20 10 20 40 2: 20 20 10 40 3: 40 40 40 10 The existing implementation gives below demotion targets, node demotion_target 0 3 1 X 2 X 3 X With this patch applied, below are the demotion targets, node demotion_target 0 3 1 3 2 3 3 X Fixes: 79c28a416722 ("mm/numa: automatically generate node migration order") Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com> Signed-off-by: Jagdish Gediya <jvgediya@linux.ibm.com> Reviewed-by: Baolin Wang <baolin.wang@linux.alibaba.com> Tested-by: Baolin Wang <baolin.wang@linux.alibaba.com> Acked-by: "Huang, Ying" <ying.huang@intel.com> --- mm/migrate.c | 25 ++++++++++++++----------- 1 file changed, 14 insertions(+), 11 deletions(-)