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 bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 00D8EC77B6D for ; Sat, 1 Apr 2023 12:51:05 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References: Message-ID:Subject:Cc:To:From:Date:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=8GAwdsha2mr/Qmls0kS6Sjy38Ahckwrg0Lz8fcwS6Us=; b=lD7v+4hvOlrTWm tb2pV0Xfp1XqdzhpQgaVO9bcF20i0Sbn8NecDgQeVBhK66B/EzMaut06wZQRfxizA261f5zY2lss+ XkgVw7ImkTuquwyHotanz8EZpL/OjuB8HRRBG6GXie1d+42MJk4UDmE6B3yykSBgF8bV1fVDJCtv6 HEkW7wS8WZB1fiNqN3QPwWoeNKPtUvtweOewz4RlOTZHRmtKDEd42eI/Ruo5fJwHFTG//HE7AczW5 i7g66D2daU+kB6kEg8tIuiGO+y72dEHqkvQxuDVEmMQ+hPFfgEAHYFgzdTuM1UoxgBpMlhacc5qrA 43tz88fl4dnTL1IYA9ZQ==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.96 #2 (Red Hat Linux)) id 1piag5-00AXcj-1Z; Sat, 01 Apr 2023 12:49:53 +0000 Received: from mail-ed1-x530.google.com ([2a00:1450:4864:20::530]) by bombadil.infradead.org with esmtps (Exim 4.96 #2 (Red Hat Linux)) id 1piag0-00AXbv-1y for linux-arm-kernel@lists.infradead.org; Sat, 01 Apr 2023 12:49:51 +0000 Received: by mail-ed1-x530.google.com with SMTP id ew6so100058659edb.7 for ; Sat, 01 Apr 2023 05:49:45 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=resnulli-us.20210112.gappssmtp.com; s=20210112; t=1680353384; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:from:to:cc:subject:date:message-id:reply-to; bh=3ewVCP+wDJHX46RNaKAZarx7ThtmmaIaa+PN5mp/uXI=; b=KjoCp7+uXnJLPnyG7tx0qBq+jW2fO0pUeHxkLnG2pfZRO45BJ2F7oFWMNPVoes+kSv SN/87ysk/WTWanPqKDYoFmasrbGoIXR3RFI5Fz/mZQ7qt7D/lbsNCU1XVfe+Ki3nQuYs BYJONtvqyweOdKlBwq8+pB9aeVIgHKkmEHppCEiy5Upltb/1QTFoIl/e9yqsY/ivBnC4 XqXUJL0N6gbMWGCvRS427wQ2bSeHLPC+7VVozrjNW4Wp2v7W2phYz4tVNpby0TTXUdnm 7VpBx4Ubong80KWZYgNHRG8OI4ofwEo/zfD5aWIAVKIiUKZAzoQE2hpHlRztcYZumvgH Fw5A== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; t=1680353384; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:x-gm-message-state:from:to:cc:subject:date :message-id:reply-to; bh=3ewVCP+wDJHX46RNaKAZarx7ThtmmaIaa+PN5mp/uXI=; b=S+pxdmLdf1k6ttyEaGhaeOtnDgnVf0j4VhEeQWBs7qCF5zVJtPpT2K0mAt1aGA8fLo JTG9pSM1EkZPapP2irych5ghmLwZ6Rg6BwYZ8DEoAMouJzlCTC0coUBX2fTAaHpZaloG dSUakenH3TgwmLgBY7X+nC2MmiEPQcmXMi2t7f7/E7npsdvCkDDGqVHsfbUqURKh7yPb xFcB96v8BRfmTbxaUpfKLndcPhh0Fn1zMo4n9aNPvLjubecneuVXkdLFce6FMFSOrC/x a6YMsCuVi4eBDutJj4bPKNy/c+G30Auk5BdRa7MxX1cQiP7EQFeaSuUuqFyEr0Bn2ET8 E+qg== X-Gm-Message-State: AAQBX9dffqDZsRUj5tT4coZmeTNMqnpom9zjH3uov2HID/Z4VDUC5vPc 97LsDHtskwdgXpSk5ukJyQm3Tw== X-Google-Smtp-Source: AKy350adyWbn7jQXnTGpzPFF4lXd+Au+soeUfPr6sJfOYwooSsz1o63aMbbl5rmMFD3s3DKr3f0MMg== X-Received: by 2002:aa7:d3cc:0:b0:502:ef8:1c80 with SMTP id o12-20020aa7d3cc000000b005020ef81c80mr31270334edr.21.1680353383732; Sat, 01 Apr 2023 05:49:43 -0700 (PDT) Received: from localhost ([86.61.181.4]) by smtp.gmail.com with ESMTPSA id f24-20020a50a6d8000000b004acbda55f6bsm2070677edc.27.2023.04.01.05.49.42 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Sat, 01 Apr 2023 05:49:42 -0700 (PDT) Date: Sat, 1 Apr 2023 14:49:41 +0200 From: Jiri Pirko To: Vadim Fedorenko Cc: "Kubalewski, Arkadiusz" , Vadim Fedorenko , Jakub Kicinski , Jonathan Lemon , Paolo Abeni , poros , mschmidt , "netdev@vger.kernel.org" , "linux-arm-kernel@lists.infradead.org" , "linux-clk@vger.kernel.org" , "Olech, Milena" , "Michalik, Michal" Subject: Re: [PATCH RFC v6 2/6] dpll: Add DPLL framework base functions Message-ID: References: <20230312022807.278528-1-vadfed@meta.com> <20230312022807.278528-3-vadfed@meta.com> <2b749045-021e-d6c8-b265-972cfa892802@linux.dev> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20230401_054948_873307_04513804 X-CRM114-Status: GOOD ( 50.13 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org Tue, Mar 28, 2023 at 05:22:04PM CEST, vadim.fedorenko@linux.dev wrote: >On 15/03/2023 09:22, Jiri Pirko wrote: >> Tue, Mar 14, 2023 at 06:50:57PM CET, arkadiusz.kubalewski@intel.com wrote: >> > > From: Jiri Pirko >> > > Sent: Tuesday, March 14, 2023 10:22 AM >> > > >> > > Mon, Mar 13, 2023 at 11:59:32PM CET, vadim.fedorenko@linux.dev wrote: >> > > > On 13.03.2023 16:21, Jiri Pirko wrote: >> > > > > Sun, Mar 12, 2023 at 03:28:03AM CET, vadfed@meta.com wrote: [...] >> > > > > > +static int >> > > > > > +dpll_msg_add_dev_handle(struct sk_buff *msg, const struct dpll_device >> > > > > > *dpll) >> > > > > > +{ >> > > > > > + if (nla_put_u32(msg, DPLL_A_ID, dpll->id)) >> > > > > >> > > > > Why exactly do we need this dua--handle scheme? Why do you need >> > > > > unpredictable DPLL_A_ID to be exposed to userspace? >> > > > > It's just confusing. >> > > > > >> > > > To be able to work with DPLL per integer after iterator on the list deducts >> > > > which DPLL device is needed. It can reduce the amount of memory copies and >> > > > simplify comparisons. Not sure why it's confusing. >> > > >> > > Wait, I don't get it. Could you please explain a bit more? >> > > >> > > My point is, there should be not such ID exposed over netlink >> > > You don't need to expose it to userspace. The user has well defined >> > > handle as you agreed with above. For example: >> > > >> > > ice/c92d02a7129f4747/1 >> > > ice/c92d02a7129f4747/2 >> > > >> > > This is shown in dpll device GET/DUMP outputs. >> > > Also user passes it during SET operation: >> > > $ dplltool set ice/c92d02a7129f4747/1 mode auto >> > > >> > > Isn't that enough stable and nice? >> > > >> > >> > I agree with Vadim, this is rather to be used by a daemon tools, which >> > would get the index once, then could use it as long as device is there. >> >> So basically you say, you can have 2 approaches in app: >> 1) >> id = dpll_device_get_id("ice/c92d02a7129f4747/1") >> dpll_device_set(id, something); >> dpll_device_set(id, something); >> dpll_device_set(id, something); >> 2): >> dpll_device_set("ice/c92d02a7129f4747/1, something); >> dpll_device_set("ice/c92d02a7129f4747/1, something); >> dpll_device_set("ice/c92d02a7129f4747/1, something); >> >> What is exactly benefit of the first one? Why to have 2 handles? Devlink >> is a nice example of 2) approach, no problem there. >> >> Perhaps I'm missing something, but looks like you want the id for no >> good reason and this dual-handle scheme just makes things more >> complicated with 0 added value. > >I would like to avoid any extra memory copies or memory checks when it's >possible to compare single u32/u64 index value. I might be invisible on a >single host setup, but running monitoring at scale which will parse and >compare string on every get/event can burn a bit of compute capacity. Wait, that does not make any sense what so ever. Show me numbers and real usecase. A sane app gets once at start, then processes notifications. You have a flow where string compare on a netlink command makes difference to int compare? Like tens of thousands of netlink cmds per second? I doubt that. If yes, it is a misuse. Btw, the string compare would be your last problem comparing the overhead of Netlink processing with ioctl for example. Your dual handle scheme just adds complexicity, confusion with 0 added value. Please drop it. I really don't understand the need to defend this odd approach :/ [...] >> > > > > > + >> > > > > > + return dpll_pre_dumpit(cb); >> > > > > > +} >> > > > > > + >> > > > > > +int dpll_pin_post_dumpit(struct netlink_callback *cb) >> > > > > > +{ >> > > > > > + mutex_unlock(&dpll_pin_xa_lock); >> > > > > > + >> > > > > > + return dpll_post_dumpit(cb); >> > > > > > +} >> > > > > > + >> > > > > > +static int >> > > > > > +dpll_event_device_change(struct sk_buff *msg, struct dpll_device >> > > > > > *dpll, >> > > > > > + struct dpll_pin *pin, struct dpll_pin *parent, >> > > > > > + enum dplla attr) >> > > > > > +{ >> > > > > > + int ret = dpll_msg_add_dev_handle(msg, dpll); >> > > > > > + struct dpll_pin_ref *ref = NULL; >> > > > > > + enum dpll_pin_state state; >> > > > > > + >> > > > > > + if (ret) >> > > > > > + return ret; >> > > > > > + if (pin && nla_put_u32(msg, DPLL_A_PIN_IDX, pin- >> > > > > > dev_driver_id)) >> > > > > > + return -EMSGSIZE; >> > > > > >> > > > > I don't really understand why you are trying figure something new and >> > > > > interesting with the change notifications. This object mix and random >> > > > > attrs fillup is something very wrong and makes userspace completely >> > > > > fuzzy about what it is getting. And yet it is so simple: >> > > > > You have 2 objects, dpll and pin, please just have: >> > > > > dpll_notify() >> > > > > dpll_pin_notify() >> > > > > and share the attrs fillup code with pin_get() and dpll_get() callbacks. >> > > > > No need for any smartness. Have this dumb and simple. >> > > > > >> > > > > Think about it more as about "object-state-snapshot" than "atomic-change" >> > > > >> > > > But with full object-snapshot user space app will lose the information about >> > > > what exactly has changed. The reason to have this event is to provide the >> > > > attributes which have changed. Otherwise, the app should have full snapshot >> > > > and >> > > > compare all attributes to figure out changes and that's might not be great >> > > > idea. >> > > >> > > Wait, are you saying that the app is stateless? Could you provide >> > > example use cases? >> > > >> > >From what I see, the app managing dpll knows the state of the device and >> > > pins, it monitors for the changes and saves new state with appropriate >> > > reaction (might be some action or maybe just log entry). >> > > >> > >> > It depends on the use case, right? App developer having those information knows >> > what has changed, thus can react in a way it thinks is most suitable. >> > IMHO from user perspective it is good to have a notification which actually >> > shows it's reason, so proper flow could be assigned to handle the reaction. >> >> Again, could you provide me specific example in which this is needed? >> I may be missing something, but I don't see how it can bring >> and benefit. It just makes the live of the app harder because it has to >> treat the get and notify messages differently. >> >> It is quite common for app to: >> init: >> 1) get object state >> 2) store it >> 3) apply configuration >> runtime: >> 1) listen to object state change >> 2) store it >> 3) apply configuration >> >> Same code for both. > >Well, I'm thinking about simple monitoring app which will wait for the events >and create an alert if the changes coming from the event differ from the >"allowed configs". In this case no real reason to store whole object state, >but the changes in events are very useful. No problem. But as I wrote elsewhere in this thread, make sure that msg format of a change message is the same as the format of get cmd message. Basically think of it as a get cmd message filtered to have only attrs which changed. Same nesting and everything. Makes it simple for the app to have same parsing code for get/dump/notification messages. [...] _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel