Message ID | 20221019095620.124909-7-alexander.atanasov@virtuozzo.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a5d:4ac7:0:0:0:0:0 with SMTP id y7csp287721wrs; Wed, 19 Oct 2022 05:12:05 -0700 (PDT) X-Google-Smtp-Source: AMsMyM6wfaTottadie7B+ZUl2BXm850ptQ1yKFvhIR2CR71Y2P2QFmqYSfJ1wCAgYBTuvKoVVYrB X-Received: by 2002:a05:6402:5485:b0:459:147a:d902 with SMTP id fg5-20020a056402548500b00459147ad902mr7402554edb.263.1666181525798; Wed, 19 Oct 2022 05:12:05 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1666181525; cv=none; d=google.com; s=arc-20160816; b=IxVuYufsks8xFOvM8SFy/pTJpKRQg60I6XF1bGeStZ3PkO5K7TczlCvuHIclbbIaNf YReqdW/SF1W0/SS0e5fTk9iQI5QhKks524KBwV/DBWwBDKGKxo/vWyeDYiQx9J6T8z1h 2YWOWGxkUckKuA57TRmce5J0C8PtyC9H0/PCDkjcznfWGSJu4fmgH2zzLLiIvj/zgM8G 8f72OF9OIbjIh/kPCXpBOOZPsu5p1CE6qlpcZd4Zkr4JeTaRbo7NnSi11YBus1Owsv/B O2rzl1dYlwFY23yBFNMl0+GPf0NqpQaM3Hq7ulyOg72bNxHeJHExqK6HHyX2UzUkmoVM 6Lcg== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:mime-version :references:in-reply-to:message-id:date:subject:cc:to:from; bh=GjBwKBFhSfpea4acgA05YoTMbnyS9dt3lQhmZ4rO6zQ=; b=n5kpIknYQiYSrdI47qqjJhNanPDDflTIvy+aXa9scNvhOZDe7VsaE2vrRpm3LTq83L +phRgihyG5JQGb5aZsHwr9N+jYtnnTShbrM1bgRdGUrFJ+SbNFpitsVsh/NsacmIZWfV YoPEf+UgXkS8cjRHWo3kPs5U4yPXSQYR6OBDLMGp2bqb2J+zOmKUHxvjfQ/FpbLRpABB dEk7fhrzxg0iL5RsL1a5LTwvEyPEqpTkh8xJz4mwPQn5qoJxPqfUzZILSw+T9gvwSHpH f6+ZFbTFeB8gzNHrShRZ4NHAH/bxLbVgsiia57Gd3QNYyxk9Rn+mk0jdFCZuGqWDkp2U +s6A== ARC-Authentication-Results: i=1; mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=fail (p=QUARANTINE sp=QUARANTINE dis=NONE) header.from=virtuozzo.com Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id hb7-20020a170907160700b0078decbc3f73si14873097ejc.460.2022.10.19.05.11.25; Wed, 19 Oct 2022 05:12:05 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; Authentication-Results: mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=fail (p=QUARANTINE sp=QUARANTINE dis=NONE) header.from=virtuozzo.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S232545AbiJSMKm (ORCPT <rfc822;samuel.l.nystrom@gmail.com> + 99 others); Wed, 19 Oct 2022 08:10:42 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:52802 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S233219AbiJSMJf (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Wed, 19 Oct 2022 08:09:35 -0400 Received: from relay.virtuozzo.com (relay.virtuozzo.com [130.117.225.111]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id DF74645F6D for <linux-kernel@vger.kernel.org>; Wed, 19 Oct 2022 04:45:31 -0700 (PDT) Received: from dev011.ch-qa.sw.ru ([172.29.1.16]) by relay.virtuozzo.com with esmtp (Exim 4.95) (envelope-from <alexander.atanasov@virtuozzo.com>) id 1ol5lf-00B8K8-EO; Wed, 19 Oct 2022 11:56:33 +0200 From: Alexander Atanasov <alexander.atanasov@virtuozzo.com> To: Nadav Amit <namit@vmware.com>, VMware PV-Drivers Reviewers <pv-drivers@vmware.com>, Arnd Bergmann <arnd@arndb.de>, Greg Kroah-Hartman <gregkh@linuxfoundation.org> Cc: kernel@openvz.org, Alexander Atanasov <alexander.atanasov@virtuozzo.com>, linux-kernel@vger.kernel.org Subject: [RFC PATCH v5 6/8] drivers: vmware: balloon - report inflated memory Date: Wed, 19 Oct 2022 12:56:18 +0300 Message-Id: <20221019095620.124909-7-alexander.atanasov@virtuozzo.com> X-Mailer: git-send-email 2.31.1 In-Reply-To: <20221019095620.124909-1-alexander.atanasov@virtuozzo.com> References: <20221019095620.124909-1-alexander.atanasov@virtuozzo.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-1.9 required=5.0 tests=BAYES_00,SPF_HELO_NONE, SPF_NONE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-getmail-retrieved-from-mailbox: =?utf-8?q?INBOX?= X-GMAIL-THRID: =?utf-8?q?1747113282608983732?= X-GMAIL-MSGID: =?utf-8?q?1747117959767673912?= |
Series |
None
|
|
Commit Message
Alexander Atanasov
Oct. 19, 2022, 9:56 a.m. UTC
Update the inflated memory in the mm core on change.
Signed-off-by: Alexander Atanasov <alexander.atanasov@virtuozzo.com>
---
drivers/misc/vmw_balloon.c | 1 +
1 file changed, 1 insertion(+)
Comments
On Wed, Oct 19, 2022 at 12:56:18PM +0300, Alexander Atanasov wrote:
> Update the inflated memory in the mm core on change.
That says what this does, but not why it is needed.
Please expand on this.
Also, is this actually fixing a bug? Is it a new feature? Something
else?
thanks,
greg k-h
On 19.10.22 13:26, Greg Kroah-Hartman wrote: > On Wed, Oct 19, 2022 at 12:56:18PM +0300, Alexander Atanasov wrote: >> Update the inflated memory in the mm core on change. > > That says what this does, but not why it is needed. > > Please expand on this. > > Also, is this actually fixing a bug? Is it a new feature? Something > else? The whole series is about adding a new feature - providing access to the balloon inflated memory amount - it's in the cover letter. Should I repeat it for every driver that implements it? I've organized it classically: - prepare - add infra - use the infra What can I improve here?
On Wed, Oct 19, 2022 at 01:38:13PM +0300, Alexander Atanasov wrote: > On 19.10.22 13:26, Greg Kroah-Hartman wrote: > > On Wed, Oct 19, 2022 at 12:56:18PM +0300, Alexander Atanasov wrote: > > > Update the inflated memory in the mm core on change. > > > > That says what this does, but not why it is needed. > > > > Please expand on this. > > > > Also, is this actually fixing a bug? Is it a new feature? Something > > else? > > The whole series is about adding a new feature - providing access to the > balloon inflated memory amount - it's in the cover letter. Should I repeat > it for every driver that implements it? Each commit needs to justify why it is needed on its own. You do not provide the needed information here at all to be able to review and understand if this commit is even correct or needed. thanks, greg k-h
On 19.10.22 13:49, Greg Kroah-Hartman wrote: > On Wed, Oct 19, 2022 at 01:38:13PM +0300, Alexander Atanasov wrote: >> On 19.10.22 13:26, Greg Kroah-Hartman wrote: >>> On Wed, Oct 19, 2022 at 12:56:18PM +0300, Alexander Atanasov wrote: >>>> Update the inflated memory in the mm core on change. >>> >>> That says what this does, but not why it is needed. >>> >>> Please expand on this. >>> >>> Also, is this actually fixing a bug? Is it a new feature? Something >>> else? >> >> The whole series is about adding a new feature - providing access to the >> balloon inflated memory amount - it's in the cover letter. Should I repeat >> it for every driver that implements it? > > Each commit needs to justify why it is needed on its own. You do not > provide the needed information here at all to be able to review and > understand if this commit is even correct or needed. Ok, understood. I will keep that in mind. Thanks.
On Oct 19, 2022, at 12:56 PM, Alexander Atanasov <alexander.atanasov@virtuozzo.com> wrote: > Update the inflated memory in the mm core on change. > > Signed-off-by: Alexander Atanasov <alexander.atanasov@virtuozzo.com> > --- > drivers/misc/vmw_balloon.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/misc/vmw_balloon.c b/drivers/misc/vmw_balloon.c > index 91d4d2a285c5..3bfd845898f5 100644 > --- a/drivers/misc/vmw_balloon.c > +++ b/drivers/misc/vmw_balloon.c > @@ -1507,6 +1507,7 @@ static void vmballoon_work(struct work_struct *work) > queue_delayed_work(system_freezable_wq, > dwork, round_jiffies_relative(HZ)); > > + balloon_set_inflated_free(atomic64_read(&b->size) << 2); > } I don’t like it in general (I think that something like that should go into some common infra). But more concretely there are at least 2 problems here. First, queueing the work should come last. Second, there are other places that change the balloon size (e.g., vmballoon_reset()), which are not handled by this patch. If you added calls to balloon_set_inflated_free() from these places, you can get races while calling balloon_set_inflated_free() and the last value that would be latched would be the wrong one. I don’t see anything in the logic or comments that clarify how something like that should be resolved.
On 21.10.22 9:50, Nadav Amit wrote: > On Oct 19, 2022, at 12:56 PM, Alexander Atanasov <alexander.atanasov@virtuozzo.com> wrote: > >> Update the inflated memory in the mm core on change. >> >> Signed-off-by: Alexander Atanasov <alexander.atanasov@virtuozzo.com> >> --- >> drivers/misc/vmw_balloon.c | 1 + >> 1 file changed, 1 insertion(+) >> >> diff --git a/drivers/misc/vmw_balloon.c b/drivers/misc/vmw_balloon.c >> index 91d4d2a285c5..3bfd845898f5 100644 >> --- a/drivers/misc/vmw_balloon.c >> +++ b/drivers/misc/vmw_balloon.c >> @@ -1507,6 +1507,7 @@ static void vmballoon_work(struct work_struct *work) >> queue_delayed_work(system_freezable_wq, >> dwork, round_jiffies_relative(HZ)); >> >> + balloon_set_inflated_free(atomic64_read(&b->size) << 2); >> } > > I don’t like it in general (I think that something like that should go into > some common infra). My goal is to create that infra, sure there is a place for improvement. I think of it as a commit point so plugging it into something existing does not fit or at least i don't see how. > But more concretely there are at least 2 problems here. First, queueing the > work should come last. Second, there are other places that change the > balloon size (e.g., vmballoon_reset()), which are not handled by this patch. > > If you added calls to balloon_set_inflated_free() from these places, you can > get races while calling balloon_set_inflated_free() and the last value that > would be latched would be the wrong one. I don’t see anything in the logic > or comments that clarify how something like that should be resolved. > Ok,I will move it before the enqueue call. But are you sure about this the reset? vmballoon_reset(...) is called only from vmballoon_work(...) which does the update ? what i am missing?
On Oct 21, 2022, at 10:25 AM, Alexander Atanasov <alexander.atanasov@virtuozzo.com> wrote: > > Ok,I will move it before the enqueue call. > But are you sure about this the reset? > vmballoon_reset(...) is called only from vmballoon_work(...) which does > the update ? what i am missing? My bad. But when the module is unloaded, vmballoon_pop() is called.
On 21.10.22 10:31, Nadav Amit wrote: > On Oct 21, 2022, at 10:25 AM, Alexander Atanasov <alexander.atanasov@virtuozzo.com> wrote: > >> >> Ok,I will move it before the enqueue call. >> But are you sure about this the reset? >> vmballoon_reset(...) is called only from vmballoon_work(...) which does >> the update ? what i am missing? > > My bad. But when the module is unloaded, vmballoon_pop() is called. Yes, i missed the unload - i will just set it to zero there.
diff --git a/drivers/misc/vmw_balloon.c b/drivers/misc/vmw_balloon.c index 91d4d2a285c5..3bfd845898f5 100644 --- a/drivers/misc/vmw_balloon.c +++ b/drivers/misc/vmw_balloon.c @@ -1507,6 +1507,7 @@ static void vmballoon_work(struct work_struct *work) queue_delayed_work(system_freezable_wq, dwork, round_jiffies_relative(HZ)); + balloon_set_inflated_free(atomic64_read(&b->size) << 2); } /**