Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933056AbcLSX0z (ORCPT ); Mon, 19 Dec 2016 18:26:55 -0500 Received: from mail.kernel.org ([198.145.29.136]:44920 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932574AbcLSX0y (ORCPT ); Mon, 19 Dec 2016 18:26:54 -0500 MIME-Version: 1.0 In-Reply-To: <20161219230713.GD2895@var.home> References: <20161219224014.GA28238@var.home> <20161219230713.GD2895@var.home> From: Paul Turner Date: Mon, 19 Dec 2016 15:26:19 -0800 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [PATCH] sched/fair: fix calc_cfs_shares fixed point arithmetics To: Samuel Thibault , Paul Turner , LKML , Peter Zijlstra , Thomas Gleixner , Ingo Molnar Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2071 Lines: 54 On Mon, Dec 19, 2016 at 3:07 PM, Samuel Thibault wrote: > Paul Turner, on Mon 19 Dec 2016 14:44:38 -0800, wrote: >> On Mon, Dec 19, 2016 at 2:40 PM, Samuel Thibault >> wrote: >> > 2159197d6677 ("sched/core: Enable increased load resolution on 64-bit kernels") >> > >> > exposed yet another miscalculation in calc_cfs_shares: MIN_SHARES is unscaled, >> > and must thus be scaled before being manipulated against "shares" amounts. >> >> It's actually intentional that MIN_SHARES is un-scaled here, this is >> necessary to support the goal of sub-partitioning groups with small >> shares. > > Uh? you mean it's normal that MIN_SHARES is here compared as such > against "shares" while e.g. in sched_group_set_shares or effective_load > it is scaled before comparing with "shares"? Yes. sched_group_set_shares() controls the amount allocated to the group. Both calc_cfs_shares() and effective_load() are subdividing this total. Which is why it is scaled up from the external value of 2. > >> E.g. A group with shares=2 and 5 threads will internally provide 2048 >> units of weight for the load-balancer to account for their >> distribution. > > But here "shares" is already scaled, so > >> > - if (shares < MIN_SHARES) >> > - shares = MIN_SHARES; > ... >> > return shares; > > This will only make sure that the returned shares is 2, not 2048. This is intentional. The MIN_SHARES you are seeing here is overloaded. Every "1" unit of share is "SCHED_LOAD_RESOLUTION" bits internally. We express a minimum of "2" in terms of the unit weight due to larger numerical errors in the "1" case. In the unscaled case this needs to be MIN_SHARES, and in the scaled case, the subdivision of the scaled values must still be >=2. To make this concrete: In this case we can then internally say that there are (internally) ~410 units of weight for each of these 5 threads. Thus, if one cpu has 4 threads and another 1, we see that as a 1640/410 imbalance, not a 2048/2048 balance. > > Samuel