From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 58589C433F5 for ; Wed, 10 Nov 2021 11:34:16 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 3C75C6113D for ; Wed, 10 Nov 2021 11:34:16 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S231148AbhKJLhC (ORCPT ); Wed, 10 Nov 2021 06:37:02 -0500 Received: from esa4.hgst.iphmx.com ([216.71.154.42]:26742 "EHLO esa4.hgst.iphmx.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S230440AbhKJLhB (ORCPT ); Wed, 10 Nov 2021 06:37:01 -0500 DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=wdc.com; i=@wdc.com; q=dns/txt; s=dkim.wdc.com; t=1636544054; x=1668080054; h=from:to:cc:subject:date:message-id:references: in-reply-to:content-id:content-transfer-encoding: mime-version; bh=E7h02d5cLbdgrgw91SRgtel1ykuw7PiSLCaLHooCVo4=; b=XIKXhaHndc9v5fTjUk6CYccWPPaGBWCbvpByWg2KMCwyLfCRpk6bf8Io EqrWCHZjBmPYwigMyGl6r2zzmihC+gQP2dPidz2GCVkZ0rHGZ4zS+KVkt 5mevgORKX40lAGCIlmWcIRfO4/s45XUpIOUayGu7aiDC6ozMuqL+UoUVn ll5kM5lpaoZemgyiyUsxq3UwHtr+uywjrxsO/jBTNEIjpgr+Ic7AYi9aC ffOLEYsNlDepU4G48SnASvu81AFqwQ4VIBrdYdsQTGmBIuePUd3gZPqxc iXon7Nmvhrh+7OSBSbhXLOXNvdliq0CFHMbXnCLR4b6rk8/gzS6/N19fM Q==; X-IronPort-AV: E=Sophos;i="5.87,223,1631548800"; d="scan'208";a="184236759" Received: from mail-bn8nam12lp2170.outbound.protection.outlook.com (HELO NAM12-BN8-obe.outbound.protection.outlook.com) ([104.47.55.170]) by ob1.hgst.iphmx.com with ESMTP; 10 Nov 2021 19:34:13 +0800 ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=By4U3g3760EdPBiteQbIK8eCTUdME0+iKvuTylIyr/UPWE/vcfvbuzaCEwdIk2+/QhEfv5pxcPWKfEjPPHQVTNDbz7UBLk0/WbfjURtyo/brf2e8+aqp1YNYXeE71smZE4/38q6cVM9nQ5Z4qhJvoDK6Pvpo573bHgvlR+/n0RLJfluaICfyFzykwsfyTrnpFB/RvC+S81q1eeEayjSsw2qR8njWGVW6UuvxRjc6VCh8VdeyRscPHKuowgWuiuR6/n2ZjdMcRVGJZPhIk7JLcogtx9cqVjxb69W14nWkI3GTlrndkok3n52ra33mRFg+AhmlWuDI9eGIykwrS3Yeqg== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=p3D46KRbPL9svHSFxcXcnB6G2iIVM+1cGvVOSpjpHP0=; b=G5ERA2yTokzO5AsULllreB3/gm5Xf1p5X/DxZa2s9oDYl/PipNegpax2MMXHsIejTd9cZvyjJP4zqjr2F18MoC0TcjOvjBeK+3hGRcuGqRFEEuZPI0xBYONtRj7DQMTejWJxs4tz4ZEGXAK7u0b8EbkTE55dhKEWAxeGK6JGCPIms1GU92UqChloxBe1Lt+xezIvlM6dMj7teImnDBnvyRy+fnzcNIeEiDU2lYlR0wRsGZq/w4BC67nfREgRRFY+M7VvoXG8WeWZRGZyDRh4KCI/Ac9RCyL2Bl2mPowkQC7Povl7hLQ1Mf9HvNZEDEjnxW6JtSszVU92B86i2kGW0A== ARC-Authentication-Results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=wdc.com; dmarc=pass action=none header.from=wdc.com; dkim=pass header.d=wdc.com; arc=none DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sharedspace.onmicrosoft.com; s=selector2-sharedspace-onmicrosoft-com; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=p3D46KRbPL9svHSFxcXcnB6G2iIVM+1cGvVOSpjpHP0=; b=uaQYlkhuhD6HauQpqxYcLAhiQANwd7za2OXEWJ93MpPOBSiob+lBPJz4xOMdsXWogd4E9cvjDoHyqfIFiYW1ZKWa/SuVBTF53FyxMNvoLHH96bgWDVEFtHTaOPoRId8zo8Ej+7VfB8UUNVsmVr5S38s4/T478NgTGqFHZFjpFyo= Received: from SJ0PR04MB7165.namprd04.prod.outlook.com (2603:10b6:a03:2a3::21) by BYAPR04MB4358.namprd04.prod.outlook.com (2603:10b6:a02:f1::21) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.4690.16; Wed, 10 Nov 2021 11:34:10 +0000 Received: from SJ0PR04MB7165.namprd04.prod.outlook.com ([fe80::1cc5:8e51:7d6e:5461]) by SJ0PR04MB7165.namprd04.prod.outlook.com ([fe80::1cc5:8e51:7d6e:5461%8]) with mapi id 15.20.4690.015; Wed, 10 Nov 2021 11:34:10 +0000 From: Niklas Cassel To: Damien Le Moal CC: "axboe@kernel.dk" , "fio@vger.kernel.org" , Damien Le Moal Subject: Re: [PATCH 7/8] cmdprio: add mode to make the logic easier to reason about Thread-Topic: [PATCH 7/8] cmdprio: add mode to make the logic easier to reason about Thread-Index: AQHX1QCwimtz2VgxCU+gnzQiTYJoIav6vtYAgABy0QCAARP5gIAAXh+A Date: Wed, 10 Nov 2021 11:34:10 +0000 Message-ID: References: <20211109002655.117274-1-Niklas.Cassel@wdc.com> <20211109002655.117274-8-Niklas.Cassel@wdc.com> <1e4cfd69-2e8a-99c7-a654-d784056c47f8@opensource.wdc.com> In-Reply-To: <1e4cfd69-2e8a-99c7-a654-d784056c47f8@opensource.wdc.com> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: authentication-results: dkim=none (message not signed) header.d=none;dmarc=none action=none header.from=wdc.com; x-ms-publictraffictype: Email x-ms-office365-filtering-correlation-id: 67113abc-4f02-4a5a-1ffd-08d9a43e03b7 x-ms-traffictypediagnostic: BYAPR04MB4358: x-microsoft-antispam-prvs: wdcipoutbound: EOP-TRUE x-ms-oob-tlc-oobclassifiers: OLM:6790; x-ms-exchange-senderadcheck: 1 x-ms-exchange-antispam-relay: 0 x-microsoft-antispam: BCL:0; x-microsoft-antispam-message-info: cVwNdsnkuEvjJzQYmOeWB+4TNWmprv6I2Pbbmdd8kmxPSimHZH91qiXJ9wSZgG6UgsjJb0k/nXN1NSbhsCwH8uD5W84uo0IMT0V5RDZ48ydt1OtV/IQTvpRYueY5m/SJaoCNcBUJpIbQfzLNrIVzcU8SLt1ssGDhhXI3fm6+AKsFc+B5//c8JPLhYS/Yxv4rk1asmtAqZcNhhWJKrzk/eIiF0RbYdT4cvmF/ZntwxWymdOnM164AphPYTA01bjcsQikWFsGJoOIpao/Xl2Vwu8XYs+YJYE0RD6MF/9TTRhJe86n3tHizOzJ2RGgiSL4mP5bsTdbejpJc7TR2Bd+AtlX1JYAPNBaSZklbAKfYglSpn3Um79lg/q4fI0WL5x2MWSMKGbWr71QulHHmXn434xEnx6hiqFq/eborfi4FUHKWgJHu2PbO/blZ9hujiFWKVEAvOUL+/AbcZ31s6un/+kAezoAzzV9Fcek7MaTVIRhDw2ReTU4CBk5+kd+0VEUyHuN3qeLADPD/PhS00iZkDN4t/Ff2kQ+flqr8TSnNUXYXV/LmPlmvwVJz4TsnRCYuW6s/urRhfbSxV23aNge3SL/I2vfDpIGiHJGHIj3mE/XlMAUgJI4MgPOefiaWIPm7knmDelN9FnvvGWZulJCSC6Kdy+tTlHL4xMtwQMXRL4njzxglfMme3UiAcg/s2LszgAAf0Aaunl+CR1DY54FmnA== x-forefront-antispam-report: CIP:255.255.255.255;CTRY:;LANG:en;SCL:1;SRV:;IPV:NLI;SFV:NSPM;H:SJ0PR04MB7165.namprd04.prod.outlook.com;PTR:;CAT:NONE;SFS:(4636009)(7916004)(366004)(91956017)(76116006)(86362001)(2906002)(6862004)(4326008)(38070700005)(9686003)(5660300002)(6512007)(33716001)(82960400001)(8676002)(54906003)(316002)(508600001)(186003)(8936002)(66446008)(83380400001)(53546011)(66556008)(6506007)(38100700002)(26005)(66476007)(71200400001)(64756008)(66946007)(122000001)(6486002);DIR:OUT;SFP:1102; x-ms-exchange-antispam-messagedata-chunkcount: 1 x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?PIqXjILGtu+0DJOta/bNdQynRtOFwIIU3mynbIoUu1Vu/tXhzE/DUDFF/puL?= =?us-ascii?Q?g/TDgGW7c1XwAqTbCBBnQc8Z1PjoPLs64WSMsNzJhC5j0bcV/7UDnFQyXavS?= =?us-ascii?Q?r71fQpvEUioOkCwISRsSxtXEU3eXyLZ+SjSO14pbzLuxdj+au3VOIxDc5d+N?= =?us-ascii?Q?hh4N7EzYL1uGir562FDRzMH90Tr3ii9QSogkp4oqxsu5cKykawpxNrJ8Nq+z?= =?us-ascii?Q?TN4SrugAwVhlqoeIVpnlwuLmsUORYaf/JxSPhyzYPFeUR0Q1w4JUWf+Rupz3?= =?us-ascii?Q?fYdYZ6EBxrrYFPvlpZ5U+tnGSdwFJS58CpsvIfkaZ+XrKkMQ8bp0pn/3ktmh?= =?us-ascii?Q?70ejXAt2J0YseogiQ4wA7pznA8NvYZQ3fuHtCFibSZwAuzzITBzPHz+44vjt?= =?us-ascii?Q?9s5JmJBSDrYWMmPBqup767ajphs9ofblAkJ4/u2u+rT0HclPRD1u+Ghmf1tZ?= =?us-ascii?Q?eURpTFDt94tWCZdX9onVYytBLoSXyL6m162A+fu43+BY0Xq9hoXFhhlEH4fW?= =?us-ascii?Q?MzVjJ7Fz5H1d9aq0Kzzvfg1eeS5ZkSWEX0/SYZxQ+4W17SK+qpbgUQIcaJ43?= =?us-ascii?Q?ZFsTJOEDB1d0dQlLWEweiqWP8T5OhfC5mN3EzgVUiIxoy62DScFqO7pL0lKD?= =?us-ascii?Q?Co3aOlu4QJ3XFhQPNOo4WwEEspe1384FsJ2aHNzF6utXL9C7sMQV+b6VCInF?= =?us-ascii?Q?FXlFcAbWX0E/q8Xb4Uo052g1poEPol5CMMuA4jx1damWC/XWHGYcrJxFYmcH?= =?us-ascii?Q?mMpAaL7xGaoaa2jtGX/khyRGNBT4io8d6XEIfgdxT8lQNM/mbRHlDuFpLw1o?= =?us-ascii?Q?c4LQA6ITE5limrI/7Yuo/9aPSB/93MWNXNFqTzsD83uwmEbb0o7MFeokV6wN?= =?us-ascii?Q?JTZtPRYZQAN5KenDg+fvyCiWmrfk14id2749zxJq7hFZQUB9p7XEIj+OSO1Y?= =?us-ascii?Q?98uO2tq4vQD+3lKQWSxIL1FqJw5+0QJeutWtEvqhtIPIrd/5nMKjFFmbLUkT?= =?us-ascii?Q?uIrXKBQKJTBbcf1NZ7QQOEbroy7hQbYzx7KyDUqeAbCT/Te1rM7AJKcKaCIj?= =?us-ascii?Q?VFYGDjUguuTV5gam9Vb6YH6Gp31VecB0ta2cm7oDrnwcJY6lPQjF4yJhJAc6?= =?us-ascii?Q?TBAQwr/o1Ujf6Mvsp+6JYrf1CQJbtIBEwRIdlENGxYNvQpKVDUA7jROaMvDV?= =?us-ascii?Q?cPJ+g+FgoVHkKHYiX9MUtA/tXGPgphdjeYdoTMSXVg0M8yDT6mfgdL4lWt/M?= =?us-ascii?Q?Dzawi5IMIZ0RKVZguvdaPocgkwC3iMBpm4HkALGSQ6O0tt/lgXZoF2FAygSl?= =?us-ascii?Q?J9svbotGP2t2cq03+1L4uFWPHWverNzBWwg0y852X8WogCRbV0iMGmGrWGTv?= =?us-ascii?Q?hyzmkUwHYOj9wBOHWsIBHTctQUJpoPgdiXqz/GnXCLMOl/2my9JW3Xpp2XJG?= =?us-ascii?Q?xQCO3nqvP/81bbtQzFaEqPVsA5HGs2YrfarIeMF7i8b8sTLjmq6jwSm9xEEj?= =?us-ascii?Q?T8Zc7vani81/YY+2fg3UH7jsqMfy8DOP1LmKJ4LAZ6tw0wR3+k6mLnnANltj?= =?us-ascii?Q?NVfwVxbfTeDQtZz9j0BdsBIVOrQuS62O5Z+V8PUkVxNSbHExRUoyy/CbNFNA?= =?us-ascii?Q?cfE+qHF/oBNIiT4X6Dzzx4BUU6lz/wQ5xwDFatcOzCGS?= Content-Type: text/plain; charset="us-ascii" Content-ID: <27E1A5E7A37EB444A1942D314B063A60@namprd04.prod.outlook.com> Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 X-OriginatorOrg: wdc.com X-MS-Exchange-CrossTenant-AuthAs: Internal X-MS-Exchange-CrossTenant-AuthSource: SJ0PR04MB7165.namprd04.prod.outlook.com X-MS-Exchange-CrossTenant-Network-Message-Id: 67113abc-4f02-4a5a-1ffd-08d9a43e03b7 X-MS-Exchange-CrossTenant-originalarrivaltime: 10 Nov 2021 11:34:10.5243 (UTC) X-MS-Exchange-CrossTenant-fromentityheader: Hosted X-MS-Exchange-CrossTenant-id: b61c8803-16f3-4c35-9b17-6f65f441df86 X-MS-Exchange-CrossTenant-mailboxtype: HOSTED X-MS-Exchange-CrossTenant-userprincipalname: e0h00aFSXwAT2TrxgqEui0u1a7ObPS0GpOecJvHM2PsKA/21QMyhXtyudUgO3g9M0j2qhrRgdkw44Wyng21g8w== X-MS-Exchange-Transport-CrossTenantHeadersStamped: BYAPR04MB4358 Precedence: bulk List-ID: X-Mailing-List: fio@vger.kernel.org On Wed, Nov 10, 2021 at 02:57:17PM +0900, Damien Le Moal wrote: > On 2021/11/09 22:29, Niklas Cassel wrote: > >>> -int fio_cmdprio_init(struct thread_data *td, struct cmdprio *cmdprio= , > >>> - bool *has_cmdprio) > >>> +int fio_cmdprio_init(struct thread_data *td, struct cmdprio *cmdprio= ) > >>> { > >>> struct thread_options *to =3D &td->o; > >>> bool has_cmdprio_percentage =3D false; > >>> @@ -140,16 +144,12 @@ int fio_cmdprio_init(struct thread_data *td, st= ruct cmdprio *cmdprio, > >>> * is not set, default to RT priority class. > >>> */ > >>> for (i =3D 0; i < CMDPRIO_RWDIR_CNT; i++) { > >>> - if (cmdprio->percentage[i]) { > >>> - if (!cmdprio->class[i]) > >>> - cmdprio->class[i] =3D IOPRIO_CLASS_RT; > >> > >> Why do you change this ? If cmdprio->percentage[i] is 0, you do not wa= nt to set > >> the calss to RT. Or is it that cmdprio->percentage[i] cannot be 0 ? (I= did not > >> check the range of the option). > > > > option cmdprio_percentage has minval 0. > > Which could be changed to 1, since having percentage =3D=3D 0 is (should = be) > equivalent to a "do not set a priority". We could change the option .minval, but it wouldn't change anything at all. Since the option is an FIO_OPT_INT, the off1 value will be 0 if the user didn't specify anything, or if the user specified 0. Either way, the code has to handle 0, so I propose that we leave it as is. > > > option cmdprio_class has minval 1, however it can still be 0 if the use= r > > omitted the option and only used option cmdprio to specify a priority l= evel. > > Hmmm... class 0 is "NONE", so no priority. In the kernel, that will be ch= anged > to the default BE class. So maybe we should do the same here to avoid thi= s weird > case ? (setting a prio level with class NONE does not make any sense). For os/os-linux.h in fio, this is how ioprio_value() looks: static inline int ioprio_value(int ioprio_class, int ioprio) { /* * If no class is set, assume BE */ if (!ioprio_class) ioprio_class =3D IOPRIO_CLASS_BE; return (ioprio_class << IOPRIO_CLASS_SHIFT) | ioprio; } So it will already use BE class when you don't specify a class. However, since the man page says that if you omit a class for cmdprio_percentage/cmdprio_bssplit, the highest priority class (RT) should be used. So for cmdprio_percentage/cmdprio_bssplit, ioprio_value() will therefore never get in class=3D=3D0. (cmdprio has already changed it t= o RT.) Therefore, the ioprio_value() function will only change class to BE when using option prio, without specifying option prioclass. Either case, since ioprio_value() is used, for prio/prioclass or an IO overloaded with a cmdprio priority value, fio will never send class=3D=3D0 to the kernel, so I think that what you are suggesting is already there. > > > > > > I was thinking that cmdprio_value is only used when the percentage !=3D= 0, > > which is the case in my cmdprio branch that allows you to have differen= t > > cmdprio class + levels, but you are right, in this specific patch > > fio_cmdprio_ioprio_was_updated()/fio_cmdprio_set_ioprio() still uses > > cmdprio->class to determine if IO_U_F_HIGH_PRIO flag should be set, > > even when fio_cmdprio_percentage() returned zero.. > > > > The question is, if percentage =3D=3D 0, for e.g. writes, > > but the user specified cmdprio=3D3 > > (which sets cmdprio->level[] for both DDIR read and write), > > should we set the HIGH_PRIO / LOW_PRIO flag? > > That is a non question... If percentage =3D=3D 0, fio_cmdprio_set_ioprio(= ) should > NOT try to change the io_u prio at all. Then the io_u HIGH/LOW flag will = depend > on the default priority. That is the same as for the 0 < percentage < 100= case > when an io_u does not get a "hit" on the percentage and fio_cmdprio_set_i= oprio() > does not change its priority. With percentage =3D=3D 0, you will never ge= t a "hit". I agree, as that is exactly what I wrote in the next sentence :) Here: > > I think the best way is to simply not set any of the flags, > > If cmdprio percentage is 0 for a certain ddir, then everything > > will be sent with the default prio. > > For libaio (io_uring is similar), the code in fio_libaio_prio_prep() is: > > if (p && rand_between(&td->prio_state, 0, 99) < p) { > io_u->ioprio =3D cmdprio_value; > io_u->iocb.aio_reqprio =3D cmdprio_value; > io_u->iocb.u.c.flags |=3D IOCB_FLAG_IOPRIO; > if (!td->ioprio || cmdprio_value < td->ioprio) { > /* > * The async IO priority is higher (has a lower v= alue) > * than the default context priority. > */ > io_u->flags |=3D IO_U_F_HIGH_PRIO; > } > } else if (td->ioprio && td->ioprio < cmdprio_value) { > /* > * The IO will be executed with the default context prior= ity, > * and this priority is higher (has a lower value) than t= he > * async IO priority. > */ > io_u->flags |=3D IO_U_F_HIGH_PRIO; > } > > So if percentage =3D=3D 0, p is 0 and the io_u flag is set according to t= he default > prio. What you are suggesting is already there and should not change. Agreed, and this is how I decided to do things in my V2 series, which is already on the fio mailing list and on your opensource.wdc.com ema= il. Please review! :) What I don't like with the existing behavior is that even when fio_cmdprio_percentage() returns zero (p =3D=3D 0), you will still perform } else if (td->ioprio && td->ioprio < cmdprio_value) { and potentially set the HIGH_PRIO flag. E.g. if you have a bssplit with both reads and writes, and cmdprio_bssplit for read DDIR has entries with non-zero values, but write DDIR only has entries with zero values. Then for write DDIR, you could still set the HIGH_PRIO flag.. even though fio_cmdprio_percentage() returns 0 for all blocksizes for write DDIR.. yucky.. Anyway, since stat.c only prints something if there is both HIGH and LOW entries, this shouldn't produce incorrect output. Anyway, I kept this existing (albeit a bit ugly, but working) behavior in my V2, but in the upcoming series, where we have a clat_prio array, we will only ever touch stuff, flags whatever, if p !=3D 0. Kind regards, Niklas=