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 kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id 513E2C25B74 for ; Sun, 2 Jun 2024 19:48:37 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 8D59C6B00B0; Sun, 2 Jun 2024 15:48:36 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 886026B00B4; Sun, 2 Jun 2024 15:48:36 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 726326B00B5; Sun, 2 Jun 2024 15:48:36 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0016.hostedemail.com [216.40.44.16]) by kanga.kvack.org (Postfix) with ESMTP id 54AA46B00B0 for ; Sun, 2 Jun 2024 15:48:36 -0400 (EDT) Received: from smtpin21.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay03.hostedemail.com (Postfix) with ESMTP id 0D50BA0369 for ; Sun, 2 Jun 2024 19:48:35 +0000 (UTC) X-FDA: 82186985790.21.C6EBD6C Received: from mail-lj1-f170.google.com (mail-lj1-f170.google.com [209.85.208.170]) by imf17.hostedemail.com (Postfix) with ESMTP id 1E63A40008 for ; Sun, 2 Jun 2024 19:48:32 +0000 (UTC) Authentication-Results: imf17.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b=IhtBaSh3; dmarc=pass (policy=none) header.from=gmail.com; spf=pass (imf17.hostedemail.com: domain of yorha.op@gmail.com designates 209.85.208.170 as permitted sender) smtp.mailfrom=yorha.op@gmail.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1717357713; h=from:from:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references:dkim-signature; bh=QQSzPio2R/73jHcq/X/sBS4Rsq01ybZmkuK5Z9CsC28=; b=hvdGirXji8ZRicpaqMkbFEwoq2Ypzdzu1TW3hIIbDakVS8gRW9f7iMMJVbca7IGpSZamAe uTYK6LDsYKD/MJ3AgekEtqkLdKdBUyS4csnJeqjFvyR2L2GhMxHz8Umvfr1ZBtxL6QUS3o l3HIDH4fDWqQvaZEKSPUw6VXx+RtYtI= ARC-Authentication-Results: i=1; imf17.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b=IhtBaSh3; dmarc=pass (policy=none) header.from=gmail.com; spf=pass (imf17.hostedemail.com: domain of yorha.op@gmail.com designates 209.85.208.170 as permitted sender) smtp.mailfrom=yorha.op@gmail.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1717357713; a=rsa-sha256; cv=none; b=LRgdKD+POVtMpB3IDbYW8hHkhETGtL4nuf8ja0JnOMx8Mhb+HxpRdMoHFuq8UCekUy0G0N NTZwN+KCBaNzvVjE0Et3qHK32+rdh7PES48t9Swo8gRB/mVZAyfuHZ/uUJFWjDKIO6MpFM oTL/aP1vlZmBI2IniSmPvbNsDZWAhtc= Received: by mail-lj1-f170.google.com with SMTP id 38308e7fff4ca-2ea903cd11bso35387011fa.1 for ; Sun, 02 Jun 2024 12:48:32 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1717357709; x=1717962509; darn=kvack.org; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:cc:to:from:from:to:cc:subject:date :message-id:reply-to; bh=QQSzPio2R/73jHcq/X/sBS4Rsq01ybZmkuK5Z9CsC28=; b=IhtBaSh3tLj2IeGNWwXWWa73lui1ZN96tKTV4jiSdXajy5WqZRd03tHb0trGF4DvZB F748JRltUXtD1pXrcQcMQxke+UP2BAueBXIOwD9eJc5yg+VwP3h8+yhSG2W52DshNl2O LdSL2dEhPRo0XIBfDQxbxtXrHLYXPIpqj20T9/8jStsPl7ZyapR19hmx2QFTNhJo6Ld2 xtD2YusFuLM3PUryPJXPlyOLag1lDyUZaywuXjU1DkK4QhEud2bx3AqFt0YpoEOKqDF+ 3FYaL1d2cYhXkE4nFXLPEn7bYRP/wwZuK5nL0RbDi+w+9+hK9zSPzS5uTT6gidrmIlyT ivew== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1717357709; x=1717962509; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:cc:to:from:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=QQSzPio2R/73jHcq/X/sBS4Rsq01ybZmkuK5Z9CsC28=; b=vxqyUJ8uD3w7az/sBnWhZFV1sLjnrciyD7PcYOOYMDuhLC2XhkSv3c9D69McQe9y+6 nbSE056GCo4dwzb/u+jBW4SfT79o9BTSffRZmHG/xdaLqoEvc+8IMQUFNyw4Ic+YLYT0 gljRe5ROm5caTuAAaVWTvVyw8HT+PlBp8iC0KGjcdcnE3FUJV/xugHjC42FAxprXtBRv 33oJgZX2hffBJjnqiJFGkDVghV0MPjhZyfnsI2CalRPjgX/p2/tePy3j4I3jURWRUze9 U9XZyB9wWtKYjc8LKziK3NpKxgw3LjQzooH2STjaZ8PwsY4nKsESVd3n2WhPLCGKbHm2 Jepw== X-Forwarded-Encrypted: i=1; AJvYcCXoLVnz7ZYXSXR0x+0+EJODp4ydvJDAutfvoWB/UBCd6kgSBdEu+oDqpsCcfi+zedH/N3Ss5bIJLfv1At3ve6GLPo8= X-Gm-Message-State: AOJu0Yxg9op05FRN8L8LpmU/73x4iN/4H254QhQVRwKNQvyj72tVdFzP D2B/VI/KLR2a2UVioDX1/VNb/42SunTX1VbLcu1/yQ3KYt5WRvRqMGmH5w== X-Google-Smtp-Source: AGHT+IGn9CG22o94zA0w4wlgAaXoMTZqKB453JEhmX2ql7S/CETojc+nreFBfGdxNBMasOPV4s/J5g== X-Received: by 2002:a2e:988f:0:b0:2e9:87b4:f483 with SMTP id 38308e7fff4ca-2ea9510814dmr50174431fa.4.1717357708992; Sun, 02 Jun 2024 12:48:28 -0700 (PDT) Received: from localhost.localdomain ([176.59.170.248]) by smtp.gmail.com with ESMTPSA id 38308e7fff4ca-2ea91cc7e19sm9677401fa.85.2024.06.02.12.48.26 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Sun, 02 Jun 2024 12:48:28 -0700 (PDT) From: Alex Rusuf To: sj@kernel.org Cc: damon@lists.linux.dev, linux-kernel@vger.kernel.org, linux-mm@kvack.org, yorha.op@gmail.com Subject: Re: [PATCH v2 2/2] mm/damon/core: implement multi-context support Date: Sun, 2 Jun 2024 22:48:14 +0300 Message-ID: <20240602194814.929457-1-yorha.op@gmail.com> X-Mailer: git-send-email 2.42.0 In-Reply-To: <20240602154848.91322-1-sj@kernel.org> References: <20240602154848.91322-1-sj@kernel.org> MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Rspamd-Queue-Id: 1E63A40008 X-Stat-Signature: pnz5i6fmf87fxgiapborq6cj7bx8qtca X-Rspam-User: X-Rspamd-Server: rspam08 X-HE-Tag: 1717357712-831191 X-HE-Meta: U2FsdGVkX19NcSYyQ46YCD02SgZo3HdgoSJ+H4Aw/RKufjNX/r4IrjhaGEA00PpCCIfsLfGBkdB/k9EV8seHMl1zjEPwBH9MyNitjR0Tvp8oMuR6E+rZTyh2CanNo8hoE2utRrlVZ5acpU2XIYdx7+rwEuycAheLvsAtao0fQ3acLtJZQdeSXGxVYnZjz+WINpZgxC1dz1uKh1h8QWslfZZf6wSV25UE6DcnyDloNFD+TypR4vQX4e2J2Jaajf7hLfkHg+RNARqxUIyPN3fPwYjawEmTStbMMazYRp/ikdj67OiV6Qm/Joal5jBXRAUhdsu9WFPxOKlwCwkEpdGDaZfcwmYVHdofDAPCWsrybCuPKMybbir7IcCdY7/sLxL0YZIaAGJfPr+k+4ITqYMCKmisu+HRBtUcrNrDVr1p4spCvw7Vm7Wv53imHelBkX3UfgSAS8eckmSKYbElJSrJdNgbqU/qA7xkJcRehJxKYhEwLN8OmfZs/nWEYB5PQmP3WqFf88cpTS/jiSGFP/JwW+rdBfCP9wuDs0g6MewgrBtDBx4Umbh//WsM9OVFIfFbp2uS7tJyZKozmKYMiBqFBQFBWQehjueAxwSRhZsPG8UEtoJ/PBryIN3gZq9Yg5fRwg+MmruBVAyVHrQ1CwJ4LRRAtn0qOOXr1BLAS0mFO3M4n7zflE4At8bOguaUvbldosH1SMOvytVee/EDZoghYmkLGuSWkkf03IC/aFUp27QJFQgG0Gi90ZrV71/7w+JzJl6J3/fgNVZ8fZi2HuJIFxA03QtRBOT7hH2Nc3JSkM/NKGmMonGHe7smoFcqDLFyqA80qYMwmyKrEcePjpW0xu8XMeLe/qOnJBmsWWku1xGeos3x5E4QDrqme4ZPQQ1oNygh8kgunbv00yyVP+1HD0gUAoxN1IdFC69Ov+0AJslHIJt6lgUTEHQ6mAI9HW1YjxgF6KO4dgnLfZkO2Qj zknocDG3 4KSqc7xTj/EltD6M0UTMqzMUajklnuv5gtz7PV3tWsdq8JcL/bifCHZuKnIoxImYf9JdjVnyYnMi8lOMt7Zey0g07y5eI/LQrv/83caKTSIMULkuhR0xSZ+MyuKGv36VHyMHtOo7jziPBZTwnNQcWiYzEdfSYvyzZcTi2F55RncfK3IzereHQeu9TE84PdLY/+8faP/RkuPdj2DoML969iWM7m1sPMZn27CBHKEKa7mC/4+3BKRHhQlzByFd/Mf0F/VzWM7NsBmLEjRt/R/ai8r7WgBc7HfzBYoUUYjCgJS4uBGtmCaFGI8+yiC2Y4TgPaKr1PL2hMUm3mbR91KEVjaK60NK+HJb8s4iGFMZrpoTMzQ50cO/PVruEsHD0oPdrrGVh1yisqlk2CjH2XVcluHKWWyPqR4O1KQEVvdpGT5eydtUYDRHUGS7JVKGjgrMloQMYSFV7GEx9TUHnHjosufQ7/vK+UhpUonJZxje3OOhWD9D2mQO19HqutK8rNY+qRVcuS6kq/tIDL+w+v4jfLHfDYg22RrDFqJ4fyUvQOFpaNUqCqjtFhHGxZ/8SWpcz9R+/ X-Bogosity: Ham, tests=bogofilter, spamicity=0.000680, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: List-Subscribe: List-Unsubscribe: [...] > > > > > > > > > > > > > /* private: */ > > > > /* for waiting until the execution of the kdamond_fn is started */ > > > > @@ -634,7 +632,10 @@ struct damon_ctx { > > > > * update > > > > */ > > > > unsigned long next_ops_update_sis; > > > > + /* upper limit for each monitoring region */ > > > > unsigned long sz_limit; > > > > + /* marker to check if context is valid */ > > > > + bool valid; > > > > > > What is the validity? > > > > This is a "flag" which indicates that the context is "valid" for kdamond > > to be able to call ->check_accesses() on it. Because we have many contexts > > we need a way to identify which context is valid while iterating over > > all of them (please, see kdamond_prepare_access_checks_ctx() function). > > It's still not very clear to me. When it is "valid" or not for kdamond to be > able to call ->check_accesses()? I assume you mean it is valid if the > monitoring operations set's ->target_valid() returns zero? > > The callback is not for preventing unnecessary ->check_accesses(), but for > knowing if continuing the monitoring makes sense or not. For example, the > callback of 'vaddr' operation set checks if the virtual address space still > exists (whether the target process is still running) Calling > ->check_accesses() for ->target_valid() returned non-zero target is totally ok, > though it is meaningless. And therefore kdamond stops if it returns non-zero. > > > > > Maybe name should be changed, > > At least some more detailed comment would be appreciated, imo. > > > but now I don't see a way how we could merge > > this into kdamond_valid_ctx() or so, because the main cause of this "valid" > > bit is that there's no more "valid" targets for this context, but also we could > > have ->after_sampling() returned something bad. > > As mentioned above, calling ->check_accesses() or others towards invalidated > targets (e.g., terminated processes's virtual address spaces) should be ok, if > any of the targets are still valid. So I don't show special technical > difficulties here. Please let me know if I'm missing something. This is true in case of only 1 context. kdamond can be stopped if there's no valid targets for this context (e.g. no address space exists anymore), but when there are many contexts we need to check if any of contexts has valid target(s). For example, we have 3 contexts per kdamond, at some point of time we have 1st context having no valid targets (process has been finished), but other 2 contexts do have valid targets, so we don't need to call ->prepare_access_checks() and ->check_accesses() as well for 1st context, but we do need to call them for other contexts. We can call ->kdamond_valid_ctx() before calling ->check_accesses(), but then we also need to check if nothing bad has been returned from ->after_sampling() call, so that we're allowed to further call ->check_accesses(). I decided to use a "valid" flag inside damon_ctx structure, so that if this context isn't valid, we will just skip it while iterating over all contexts. In case of just 1 context kdamond_prepare_access_checks() will return false, so that nr_valid_ctxs will remain zero, so we will break "while" loop, otherwise we'll see that there's at least one valid context to call ->check_accesses() for. The last point is that we need to understand for which context we can call ->check_accesses(), this is done by checking ctx->valid bit, if it is "false" (we already called kdamond_valid_ctx() and ->after_sampling(), and one of this calls failed) we just skip this context by doing "goto next_ctx;" in main kdamond loop. > > > > > > > > > > > > > > /* public: */ > > > > struct kdamond *kdamond; > > > > @@ -682,6 +683,12 @@ static inline struct damon_ctx *damon_first_ctx(struct kdamond *kdamond) > > > > return list_first_entry(&kdamond->contexts, struct damon_ctx, list); > > > > } > > > > > > > > +static inline bool damon_is_last_ctx(struct damon_ctx *ctx, > > > > + struct kdamond *kdamond) > > > > +{ > > > > + return list_is_last(&ctx->list, &kdamond->contexts); > > > > +} > > > > + > > > > #define damon_for_each_region(r, t) \ > > > > list_for_each_entry(r, &t->regions_list, list) > > > > > > > > diff --git a/include/trace/events/damon.h b/include/trace/events/damon.h > > > > index 23200aabc..d5287566c 100644 > > > > --- a/include/trace/events/damon.h > > > > +++ b/include/trace/events/damon.h > > > > > > Let's separate this change to another patch. > > > > Separating patches we hardly be able to reach at least build > > consistency between patches. Moreover DAMON won't be able > > to function corretly in between. > > I agree that it's not very easy, indeed. But let's try. In terms of > functionality, we need to keep the old behavior that visible to users. For > example, this change tries to make the traceevent changed for the multi > contexts support. It is for making the behavior _better_, not for keeping old > behavior. Rather than that, this is introducing a new change to the tracepoint > output. Just make no change here. Users may get confused when they use > multiple contexts, but what they see is not changed. > > Further, you can delay letting users (user-space) using the multiple contexts > (allowing >1 input to nr_contexts of DAMON sysfs interface) after making this > change in a separate patch. > > > > > > [...] > > > > > > > > -static void damos_apply_scheme(struct damon_ctx *c, struct damon_target *t, > > > > - struct damon_region *r, struct damos *s) > > > > +static void damos_apply_scheme(unsigned int cidx, struct damon_ctx *c, > > > > + struct damon_target *t, struct damon_region *r, > > > > + struct damos *s) > > > > > > Unnecesary change. > > > > What do you mean? Why is this unnecessary? Now DAMON iterates over > > contexts and calls kdamond_apply_schemes(ctx), so how can we know > > which context we print trace for? Sorry, maybe I misunderstood > > how DAMON does it, but I'm a bit confused. > > I believe the above comment to tracevent change explains this. Just to make it clear, you mean separating this change into different smaller patch? [...] BR, Alex