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 vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 88CB7E7F15A for ; Wed, 27 Sep 2023 03:42:58 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S229522AbjI0Dm5 (ORCPT ); Tue, 26 Sep 2023 23:42:57 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:36230 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S230156AbjI0DgT (ORCPT ); Tue, 26 Sep 2023 23:36:19 -0400 Received: from mgamail.intel.com (mgamail.intel.com [134.134.136.24]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id E6C3A16E90; Tue, 26 Sep 2023 18:56:46 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1695779807; x=1727315807; h=to:cc:subject:references:date:mime-version: content-transfer-encoding:from:message-id:in-reply-to; bh=B4V8UJd1VttRqhaNTVjHfz83p7oeLXpMegVFKtP+eNI=; b=UxZtPrFhuK534xBdP0e4E4LeFaPVRj7IZ8qoYMbOkc5f7KJguxJE2sCM O1kilq1J4ZIbAmyX/EPVdEDAdehmKkwWuSCJNr6g2PmSqkQjX1mUXomnZ Mva4bMlHKR171LZwDpFQymnGENaT/lcI7WK6bySsk/urxR803AeCg6XXQ hV5ZUAhk2NBxGLRkw3oe/1tn2WGIhtGrnu51l5dN7WaE/sMHivf7Q23O0 pjml3dhCCltjIv/Uac334GapizwZkQMkyCZbSmOzlw7BzS0esBiyIpDbH 2vFX6VTVLIbs0swVKuiLWQq4bEkHVWbZZ1dXPSFgCNZoSftmN0W6E+EQE w==; X-IronPort-AV: E=McAfee;i="6600,9927,10845"; a="384488449" X-IronPort-AV: E=Sophos;i="6.03,179,1694761200"; d="scan'208";a="384488449" Received: from orsmga001.jf.intel.com ([10.7.209.18]) by orsmga102.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 26 Sep 2023 18:56:46 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=McAfee;i="6600,9927,10845"; a="784156380" X-IronPort-AV: E=Sophos;i="6.03,179,1694761200"; d="scan'208";a="784156380" Received: from hhuan26-mobl.amr.corp.intel.com ([10.92.96.100]) by orsmga001-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-SHA; 26 Sep 2023 18:56:44 -0700 Content-Type: text/plain; charset=iso-8859-15; format=flowed; delsp=yes To: "Jarkko Sakkinen" , dave.hansen@linux.intel.com, tj@kernel.org, linux-kernel@vger.kernel.org, linux-sgx@vger.kernel.org, x86@kernel.org, cgroups@vger.kernel.org, tglx@linutronix.de, mingo@redhat.com, bp@alien8.de, hpa@zytor.com, sohil.mehta@intel.com Cc: zhiquan1.li@intel.com, kristen@linux.intel.com, seanjc@google.com, zhanb@microsoft.com, anakrish@microsoft.com, mikko.ylinen@linux.intel.com, yangjie@microsoft.com Subject: Re: [PATCH v5 01/18] cgroup/misc: Add per resource callbacks for CSS events References: <20230923030657.16148-1-haitao.huang@linux.intel.com> <20230923030657.16148-2-haitao.huang@linux.intel.com> Date: Tue, 26 Sep 2023 20:56:43 -0500 MIME-Version: 1.0 Content-Transfer-Encoding: 7bit From: "Haitao Huang" Organization: Intel Message-ID: In-Reply-To: User-Agent: Opera Mail/1.0 (Win32) Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, 26 Sep 2023 08:13:18 -0500, Jarkko Sakkinen wrote: ... >> > >> /** >> > >> @@ -410,7 +429,14 @@ misc_cg_alloc(struct cgroup_subsys_state >> > >> *parent_css) >> > >> */ >> > >> static void misc_cg_free(struct cgroup_subsys_state *css) >> > >> { >> > >> - kfree(css_misc(css)); >> > >> + struct misc_cg *cg = css_misc(css); >> > >> + enum misc_res_type i; >> > >> + >> > >> + for (i = 0; i < MISC_CG_RES_TYPES; i++) >> > >> + if (cg->res[i].free) >> > >> + cg->res[i].free(cg); >> > >> + >> > >> + kfree(cg); >> > >> } >> > >> >> > >> /* Cgroup controller callbacks */ >> > >> -- >> > >> 2.25.1 >> > > >> > > Since the only existing client feature requires all callbacks, >> should >> > > this not have that as an invariant? >> > > >> > > I.e. it might be better to fail unless *all* ops are non-nil (e.g. >> to >> > > catch issues in the kernel code). >> > > >> > >> > These callbacks are chained from cgroup_subsys, and they are defined >> > separately so it'd be better follow the same pattern. Or change >> together >> > with cgroup_subsys if we want to do that. Reasonable? >> >> I noticed this one later: >> >> It would better to create a separate ops struct and declare the instance >> as const at minimum. >> >> Then there is no need for dynamic assigment of ops and all that is in >> rodata. This is improves both security and also allows static analysis >> bit better. >> >> Now you have to dynamically trace the struct instance, e.g. in case of >> a bug. If this one done, it would be already in the vmlinux. >I.e. then in the driver you can have static const struct declaration > with *all* pointers pre-assigned. > > Not sure if cgroups follows this or not but it is *objectively* > better. Previous work is not always best possible work... > IIUC, like vm_ops field in vma structs. Although function pointers in vm_ops are assigned statically, but you still need dynamically assign vm_ops for each instance of vma. So the code will look like this: if (parent_cg->res[i].misc_ops && parent_cg->res[i].misc_ops->alloc) { ... } I don't see this is the pattern used in cgroups and no strong opinion either way. TJ, do you have preference on this? Thanks Haitao