Message-ID: <979da62f-3103-346a-c1f0-2ea4f0ba37bd@bytedance.com>
Date:   Fri, 17 Feb 2023 18:40:03 +0800
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:102.0)
 Gecko/20100101 Thunderbird/102.7.2
Subject: Re: [PATCH v5 2/2] sched/fair: Introduce SIS_SHORT to wake up short
 task on current CPU
Content-Language: en-US
To:     Honglei Wang <wanghonglei@didichuxing.com>,
        Chen Yu <yu.c.chen@intel.com>,
        Peter Zijlstra <peterz@infradead.org>,
        Vincent Guittot <vincent.guittot@linaro.org>,
        Ingo Molnar <mingo@redhat.com>,
        Juri Lelli <juri.lelli@redhat.com>
Cc:     Mel Gorman <mgorman@techsingularity.net>,
        Tim Chen <tim.c.chen@intel.com>,
        Dietmar Eggemann <dietmar.eggemann@arm.com>,
        Steven Rostedt <rostedt@goodmis.org>,
        Ben Segall <bsegall@google.com>,
        K Prateek Nayak <kprateek.nayak@amd.com>,
        Yicong Yang <yangyicong@hisilicon.com>,
        "Gautham R . Shenoy" <gautham.shenoy@amd.com>,
        Len Brown <len.brown@intel.com>,
        Chen Yu <yu.chen.surf@gmail.com>,
        Tianchen Ding <dtcccc@linux.alibaba.com>,
        Joel Fernandes <joel@joelfernandes.org>,
        Josh Don <joshdon@google.com>, Hillf Danton <hdanton@sina.com>,
        linux-kernel@vger.kernel.org,
        kernel test robot <yujie.liu@intel.com>
References: <f88838ff-2024-ca32-069e-f7a4c0465961@didichuxing.com>
From:   Abel Wu <wuyun.abel@bytedance.com>
In-Reply-To: <f88838ff-2024-ca32-069e-f7a4c0465961@didichuxing.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk

Hi Honglei,

On 2/17/23 4:35 PM, Honglei Wang wrote:
>> The following change greatly reduced the p99lat of Redis service
>> from 150ms to 0.9ms, at exactly the same throughput (QPS).
>>
>> @@ -5763,6 +5787,9 @@ wake_affine_weight(struct sched_domain *sd, 
>> struct task_struct *p,
>>      s64 this_eff_load, prev_eff_load;
>>      unsigned long task_load;
>>
>> +    if (is_short_task(p))
>> +        return nr_cpumask_bits;
>> +
>>      this_eff_load = cpu_load(cpu_rq(this_cpu));
>>
>>      if (sync) {
>>
>> I know that 'short' tasks are not necessarily 'small' tasks, e.g.
>> sleeping duration is small or have large weights, but this works
>> really well for this case. This is partly because delivering data
>> is memory bandwidth intensive hence prefer cache hot cpus. And I
>> think this is also applicable to the general purposes: do NOT let
>> the short running tasks suffering from cache misses caused by
>> migration.
>>
> 
> Redis is a bit special. It runs quick and really sensitive on schedule 
> latency. The purpose of this 'short task' feature from Yu is to mitigate 
> the migration and tend to place the waking task on local cpu, this is 
> somehow on the opposite side of workload such as Redis. The changes you 
> did remind me of the latency-prio stuff. Maybe we can do something base 
> on both the 'short task' and 'latency-prio' to make your changes more 
> general. thoughts?

I think it is more like an enhance rather than conflict. Chen Yu's patch
treats the cpus with only one short task as idle, to make idle cpu scan
more efficient. So if this cpu is such 'idle' cpu, just choose it. While
what I suggested is to ignore this cpu if it is not idle.

But as you pointed out that Redis is a bit special in the manner of
sensitive on scheduling latency, the change in wake_affine_weight() may
be inappropriate as 'weight' implies more on throughput than latency.

Best Regards,
	Abel