Message ID | 20230315113133.11326-5-kirill.shutemov@linux.intel.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a5d:5915:0:0:0:0:0 with SMTP id v21csp2275267wrd; Wed, 15 Mar 2023 04:38:46 -0700 (PDT) X-Google-Smtp-Source: AK7set+fyO8ruyEukkcLrdp3NzVaFfP9L92OnO0AcljV71vlpcBsxzbGMkF87SP/Z4QUXoUD/meu X-Received: by 2002:a05:6a20:4f10:b0:d3:d236:f5b7 with SMTP id gi16-20020a056a204f1000b000d3d236f5b7mr11352378pzb.26.1678880326264; Wed, 15 Mar 2023 04:38:46 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1678880326; cv=none; d=google.com; s=arc-20160816; b=Ga1XPkyW6vref/LQgXZ9w2XZjgco1H0R39CVtCW+ObV/u2WfqA+4XG5cB97n+/FI1u 1jhFa2Dco8du9uttqmogEsF2CHDscVuysCyKta2IhX5uet44jq+jqf13fnd4+azKX1B8 yMDVKKBAeNnzbaT4sQTsPbg0VrWHirlklFNfEDT/qIEcaSeI63ZSi6osvmygO12LTCUW 2huXvqO+EPJ6Vt+RMhSkhTQCiJriVtsJyYBuB4pAST5KWaI/xERmNB9nL9/IQZy+IJOU akLmWcnRuhYfkfHOpEQ3inf/TDCIdQEDqOR3HwKeXznNTFFhNrnbqkOQqX57nIflFCWJ lVhA== 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 :dkim-signature; bh=U4pgMkKqMw8kMXqqLc84IGGloFN7AbtYOBFMOlaSdh8=; b=ysOyMxAkKAJgNQWbL9DGS6k0ovXcZ4FjJEFOz0nSdARpt3sBcHsvpnA+vXw6bprMvw yv58Gv5gl7VvAtb/wrhijlQzDqykp7417iRSAy3YqG2XoVEdTw+ciUEAS1KOH3XkkOJV 6BEjVeWUTUz7Z6F6uMuY2RREUZKg2yZC2izwz5lJ6EOCCAZMbw5QalawUVcIT+a/hODA /OrU53Kx71sZdjI6dhZn8hxUPs8zc0DjMpAiMhXvyjzo0OIXZk40O6PZYxquHf1YsTiD vK10k0JH3+rzSIl/p0d3aYyAQ69xezPfIpIlegOsxPPUJpUT9vWzPeJtuMmKpW3EcpDg 4lKQ== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@intel.com header.s=Intel header.b=koXTKplP; 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=pass (p=NONE sp=NONE dis=NONE) header.from=intel.com Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id 37-20020a630c65000000b004fb20bf5812si4620714pgm.729.2023.03.15.04.38.20; Wed, 15 Mar 2023 04:38:46 -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; dkim=pass header.i=@intel.com header.s=Intel header.b=koXTKplP; 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=pass (p=NONE sp=NONE dis=NONE) header.from=intel.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S231773AbjCOLcA (ORCPT <rfc822;ruipengqi7@gmail.com> + 99 others); Wed, 15 Mar 2023 07:32:00 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:59498 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S231563AbjCOLbq (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Wed, 15 Mar 2023 07:31:46 -0400 Received: from mga03.intel.com (mga03.intel.com [134.134.136.65]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 9C898637EB; Wed, 15 Mar 2023 04:31:45 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1678879905; x=1710415905; h=from:to:cc:subject:date:message-id:in-reply-to: references:mime-version:content-transfer-encoding; bh=iwb00sFFSTPogcirxHway05JjoMe28eGhip2vpTL5qE=; b=koXTKplPFP8v3n3Kd+PxTUwBvXmaAjfVoESvEmqtRU6xNetdFIQEfXyv bK/oJtj9g3aN0R3lKi1YYcT40J0MZ1wt08AHFjGmu2aJP7PFUrpdJuqZP 66QcbOtDGLaJAMPLWjAWY7H9V4gxeZGjB9vuQ49HQsjSS0K/ughe85jcT FwzUWmkY5/Qbr8CyP5pbAlnTCIexsH+h5ekLF+Vs2J17Ki1DpaVv4tq2s Ur/DGd/5xvgZ3SoXX7xjnhC1GYFtNyTzmAC8G3KgGMqjNZRVWAO71zl0c RExxRKGiQGxxWK7B+EWLiiiSpRY+efKI+JxfH6vS3MSUgE1jG5e7Ihrqd A==; X-IronPort-AV: E=McAfee;i="6500,9779,10649"; a="340040105" X-IronPort-AV: E=Sophos;i="5.98,262,1673942400"; d="scan'208";a="340040105" Received: from fmsmga003.fm.intel.com ([10.253.24.29]) by orsmga103.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 15 Mar 2023 04:31:42 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=McAfee;i="6500,9779,10649"; a="768456009" X-IronPort-AV: E=Sophos;i="5.98,262,1673942400"; d="scan'208";a="768456009" Received: from nopopovi-mobl1.ger.corp.intel.com (HELO box.shutemov.name) ([10.252.33.48]) by fmsmga003-auth.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 15 Mar 2023 04:31:38 -0700 Received: by box.shutemov.name (Postfix, from userid 1000) id C230110CCA0; Wed, 15 Mar 2023 14:31:35 +0300 (+03) From: "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com> To: Andrew Morton <akpm@linux-foundation.org>, Mel Gorman <mgorman@suse.de>, Vlastimil Babka <vbabka@suse.cz>, David Hildenbrand <david@redhat.com> Cc: linux-mm@kvack.org, linux-arch@vger.kernel.org, linux-kernel@vger.kernel.org, "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>, Jani Nikula <jani.nikula@linux.intel.com>, Joonas Lahtinen <joonas.lahtinen@linux.intel.com>, Rodrigo Vivi <rodrigo.vivi@intel.com>, Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> Subject: [PATCH 04/10] drm/i915: Fix MAX_ORDER usage in i915_gem_object_get_pages_internal() Date: Wed, 15 Mar 2023 14:31:27 +0300 Message-Id: <20230315113133.11326-5-kirill.shutemov@linux.intel.com> X-Mailer: git-send-email 2.39.2 In-Reply-To: <20230315113133.11326-1-kirill.shutemov@linux.intel.com> References: <20230315113133.11326-1-kirill.shutemov@linux.intel.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-4.3 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_EF,RCVD_IN_DNSWL_MED,SPF_HELO_NONE, SPF_NONE,URIBL_BLOCKED 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?1760433616918703061?= X-GMAIL-MSGID: =?utf-8?q?1760433616918703061?= |
Series |
Fix confusion around MAX_ORDER
|
|
Commit Message
Kirill A. Shutemov
March 15, 2023, 11:31 a.m. UTC
MAX_ORDER is not inclusive: the maximum allocation order buddy allocator
can deliver is MAX_ORDER-1.
Fix MAX_ORDER usage in i915_gem_object_get_pages_internal().
Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
Cc: Jani Nikula <jani.nikula@linux.intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
---
drivers/gpu/drm/i915/gem/i915_gem_internal.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
Comments
On 15/03/2023 11:31, Kirill A. Shutemov wrote: > MAX_ORDER is not inclusive: the maximum allocation order buddy allocator > can deliver is MAX_ORDER-1. This looks to be true on inspection: __alloc_pages(): .. if (WARN_ON_ONCE_GFP(order >= MAX_ORDER, gfp)) So a bit of a misleading name "max".. For the i915 patch: Acked-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> I don't however see the whole series to understand the context, or how you want to handle the individual patches. Is it a tree wide cleanup of the same mistake? Regards, Tvrtko > Fix MAX_ORDER usage in i915_gem_object_get_pages_internal(). > > Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com> > Cc: Jani Nikula <jani.nikula@linux.intel.com> > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> > --- > drivers/gpu/drm/i915/gem/i915_gem_internal.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_internal.c b/drivers/gpu/drm/i915/gem/i915_gem_internal.c > index 6bc26b4b06b8..eae9e9f6d3bf 100644 > --- a/drivers/gpu/drm/i915/gem/i915_gem_internal.c > +++ b/drivers/gpu/drm/i915/gem/i915_gem_internal.c > @@ -36,7 +36,7 @@ static int i915_gem_object_get_pages_internal(struct drm_i915_gem_object *obj) > struct sg_table *st; > struct scatterlist *sg; > unsigned int npages; /* restricted by sg_alloc_table */ > - int max_order = MAX_ORDER; > + int max_order = MAX_ORDER - 1; > unsigned int max_segment; > gfp_t gfp; >
On Wed, Mar 15, 2023 at 02:18:52PM +0000, Tvrtko Ursulin wrote: > > On 15/03/2023 11:31, Kirill A. Shutemov wrote: > > MAX_ORDER is not inclusive: the maximum allocation order buddy allocator > > can deliver is MAX_ORDER-1. > > This looks to be true on inspection: > > __alloc_pages(): > .. > if (WARN_ON_ONCE_GFP(order >= MAX_ORDER, gfp)) > > So a bit of a misleading name "max".. For the i915 patch: > > Acked-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > > I don't however see the whole series to understand the context, or how you > want to handle the individual patches. Is it a tree wide cleanup of the same > mistake? The whole patchset can be seen here: https://lore.kernel.org/all/20230315113133.11326-1-kirill.shutemov@linux.intel.com/ The idea is to fix all MAX_ORDER bugs first and then re-define MAX_ORDER more sensibly.
On 15/03/2023 15:28, Kirill A. Shutemov wrote: > On Wed, Mar 15, 2023 at 02:18:52PM +0000, Tvrtko Ursulin wrote: >> >> On 15/03/2023 11:31, Kirill A. Shutemov wrote: >>> MAX_ORDER is not inclusive: the maximum allocation order buddy allocator >>> can deliver is MAX_ORDER-1. >> >> This looks to be true on inspection: >> >> __alloc_pages(): >> .. >> if (WARN_ON_ONCE_GFP(order >= MAX_ORDER, gfp)) >> >> So a bit of a misleading name "max".. For the i915 patch: >> >> Acked-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >> >> I don't however see the whole series to understand the context, or how you >> want to handle the individual patches. Is it a tree wide cleanup of the same >> mistake? > > The whole patchset can be seen here: > > https://lore.kernel.org/all/20230315113133.11326-1-kirill.shutemov@linux.intel.com/ > > The idea is to fix all MAX_ORDER bugs first and then re-define MAX_ORDER > more sensibly. Sounds good. Would you like i915 to take this patch or you will be bringing the whole lot via some other route? Former is okay and latter should also be fine for i915 since I don't envisage any conflicts here. Regards, Tvrtko
On Wed, Mar 15, 2023 at 03:35:23PM +0000, Tvrtko Ursulin wrote: > > On 15/03/2023 15:28, Kirill A. Shutemov wrote: > > On Wed, Mar 15, 2023 at 02:18:52PM +0000, Tvrtko Ursulin wrote: > > > > > > On 15/03/2023 11:31, Kirill A. Shutemov wrote: > > > > MAX_ORDER is not inclusive: the maximum allocation order buddy allocator > > > > can deliver is MAX_ORDER-1. > > > > > > This looks to be true on inspection: > > > > > > __alloc_pages(): > > > .. > > > if (WARN_ON_ONCE_GFP(order >= MAX_ORDER, gfp)) > > > > > > So a bit of a misleading name "max".. For the i915 patch: > > > > > > Acked-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > > > > > > I don't however see the whole series to understand the context, or how you > > > want to handle the individual patches. Is it a tree wide cleanup of the same > > > mistake? > > > > The whole patchset can be seen here: > > > > https://lore.kernel.org/all/20230315113133.11326-1-kirill.shutemov@linux.intel.com/ > > > > The idea is to fix all MAX_ORDER bugs first and then re-define MAX_ORDER > > more sensibly. > > Sounds good. > > Would you like i915 to take this patch or you will be bringing the whole lot > via some other route? Former is okay and latter should also be fine for i915 > since I don't envisage any conflicts here. I think would be better to get it via mm tree.
On 15/03/2023 15:38, Kirill A. Shutemov wrote: > On Wed, Mar 15, 2023 at 03:35:23PM +0000, Tvrtko Ursulin wrote: >> >> On 15/03/2023 15:28, Kirill A. Shutemov wrote: >>> On Wed, Mar 15, 2023 at 02:18:52PM +0000, Tvrtko Ursulin wrote: >>>> >>>> On 15/03/2023 11:31, Kirill A. Shutemov wrote: >>>>> MAX_ORDER is not inclusive: the maximum allocation order buddy allocator >>>>> can deliver is MAX_ORDER-1. >>>> >>>> This looks to be true on inspection: >>>> >>>> __alloc_pages(): >>>> .. >>>> if (WARN_ON_ONCE_GFP(order >= MAX_ORDER, gfp)) >>>> >>>> So a bit of a misleading name "max".. For the i915 patch: >>>> >>>> Acked-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >>>> >>>> I don't however see the whole series to understand the context, or how you >>>> want to handle the individual patches. Is it a tree wide cleanup of the same >>>> mistake? >>> >>> The whole patchset can be seen here: >>> >>> https://lore.kernel.org/all/20230315113133.11326-1-kirill.shutemov@linux.intel.com/ >>> >>> The idea is to fix all MAX_ORDER bugs first and then re-define MAX_ORDER >>> more sensibly. >> >> Sounds good. >> >> Would you like i915 to take this patch or you will be bringing the whole lot >> via some other route? Former is okay and latter should also be fine for i915 >> since I don't envisage any conflicts here. > > I think would be better to get it via mm tree. Ack for that. But as I saw that by the end of the series you also change this back as you redefine MAX_ORDER to be inclusive you could even simplify things and just not do anything for i915. I am pretty sure we never call this helper for > 4M allocations otherwise we would have seen this warn. Regards, Tvrtko
On 3/15/23 12:31, Kirill A. Shutemov wrote: > MAX_ORDER is not inclusive: the maximum allocation order buddy allocator > can deliver is MAX_ORDER-1. > > Fix MAX_ORDER usage in i915_gem_object_get_pages_internal(). > > Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com> Acked-by: Vlastimil Babka <vbabka@suse.cz> > Cc: Jani Nikula <jani.nikula@linux.intel.com> > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> > --- > drivers/gpu/drm/i915/gem/i915_gem_internal.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_internal.c b/drivers/gpu/drm/i915/gem/i915_gem_internal.c > index 6bc26b4b06b8..eae9e9f6d3bf 100644 > --- a/drivers/gpu/drm/i915/gem/i915_gem_internal.c > +++ b/drivers/gpu/drm/i915/gem/i915_gem_internal.c > @@ -36,7 +36,7 @@ static int i915_gem_object_get_pages_internal(struct drm_i915_gem_object *obj) > struct sg_table *st; > struct scatterlist *sg; > unsigned int npages; /* restricted by sg_alloc_table */ > - int max_order = MAX_ORDER; > + int max_order = MAX_ORDER - 1; > unsigned int max_segment; > gfp_t gfp; >
On 3/16/23 09:55, Tvrtko Ursulin wrote: > > On 15/03/2023 15:38, Kirill A. Shutemov wrote: >> On Wed, Mar 15, 2023 at 03:35:23PM +0000, Tvrtko Ursulin wrote: >>> >>> On 15/03/2023 15:28, Kirill A. Shutemov wrote: >>>> On Wed, Mar 15, 2023 at 02:18:52PM +0000, Tvrtko Ursulin wrote: >>>>> >>>>> On 15/03/2023 11:31, Kirill A. Shutemov wrote: >>>>>> MAX_ORDER is not inclusive: the maximum allocation order buddy allocator >>>>>> can deliver is MAX_ORDER-1. >>>>> >>>>> This looks to be true on inspection: >>>>> >>>>> __alloc_pages(): >>>>> .. >>>>> if (WARN_ON_ONCE_GFP(order >= MAX_ORDER, gfp)) >>>>> >>>>> So a bit of a misleading name "max".. For the i915 patch: >>>>> >>>>> Acked-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >>>>> >>>>> I don't however see the whole series to understand the context, or how you >>>>> want to handle the individual patches. Is it a tree wide cleanup of the same >>>>> mistake? >>>> >>>> The whole patchset can be seen here: >>>> >>>> https://lore.kernel.org/all/20230315113133.11326-1-kirill.shutemov@linux.intel.com/ >>>> >>>> The idea is to fix all MAX_ORDER bugs first and then re-define MAX_ORDER >>>> more sensibly. >>> >>> Sounds good. >>> >>> Would you like i915 to take this patch or you will be bringing the whole lot >>> via some other route? Former is okay and latter should also be fine for i915 >>> since I don't envisage any conflicts here. >> >> I think would be better to get it via mm tree. > > Ack for that. But as I saw that by the end of the series you also change > this back as you redefine MAX_ORDER to be inclusive you could even > simplify things and just not do anything for i915. I am pretty sure we > never call this helper for > 4M allocations otherwise we would have seen > this warn. I think it's better the Kirill's way as the redefinition patch then isn't also a silent bugfix. In case some of the bugfixes would need to be backported to stable (maybe you don't seen the warn, but something else will change and start seeing it?), it's better if they are separate. > Regards, > > Tvrtko >
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_internal.c b/drivers/gpu/drm/i915/gem/i915_gem_internal.c index 6bc26b4b06b8..eae9e9f6d3bf 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_internal.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_internal.c @@ -36,7 +36,7 @@ static int i915_gem_object_get_pages_internal(struct drm_i915_gem_object *obj) struct sg_table *st; struct scatterlist *sg; unsigned int npages; /* restricted by sg_alloc_table */ - int max_order = MAX_ORDER; + int max_order = MAX_ORDER - 1; unsigned int max_segment; gfp_t gfp;