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;
From:   Song Liu <songliubraving@fb.com>
To:     "Edgecombe, Rick P" <rick.p.edgecombe@intel.com>
CC:     "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "daniel@iogearbox.net" <daniel@iogearbox.net>,
        "peterz@infradead.org" <peterz@infradead.org>,
        "ast@kernel.org" <ast@kernel.org>,
        "bpf@vger.kernel.org" <bpf@vger.kernel.org>,
        "Torvalds, Linus" <torvalds@linux-foundation.org>,
        Kernel Team <Kernel-team@fb.com>,
        "song@kernel.org" <song@kernel.org>,
        "mcgrof@kernel.org" <mcgrof@kernel.org>
Subject: Re: [PATCH bpf-next 5/5] bpf: use module_alloc_huge for bpf_prog_pack
Thread-Topic: [PATCH bpf-next 5/5] bpf: use module_alloc_huge for
 bpf_prog_pack
Thread-Index: AQHYaOeMGoBl3hFoL0eR1OYIkVdnbK0jcwCAgAAfoYCAAC+jgIACAzQAgACrl4CAABjOAA==
Date:   Thu, 19 May 2022 18:25:39 +0000
Message-ID: <8C89EED6-6531-4144-BDA8-B6519396F67C@fb.com>
References: <20220516054051.114490-1-song@kernel.org>
 <20220516054051.114490-6-song@kernel.org>
 <83a69976cb93e69c5ad7a9511b5e57c402eee19d.camel@intel.com>
 <68615225-D09D-465A-8EEC-6F81EF074854@fb.com>
 <dc23afb892846ef41d73a41d58c07f6620cb6312.camel@intel.com>
 <E0C04599-E7E0-4377-8826-74FA073FC631@fb.com>
 <d3de84ce7fa62c9e460a49143ffa4709b6351390.camel@intel.com>
In-Reply-To: <d3de84ce7fa62c9e460a49143ffa4709b6351390.camel@intel.com>
Accept-Language: en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?m6h/2vPF02xjF9BZ70Z7PzJQM2lD1XFHlkrjTsNbR9L4b8+TOV01iknpS0f0?=
 =?us-ascii?Q?tY4RHuxjoENvmtpWF8vc0Qm5YmvwDoHwVI613SVtDYUj9OXB5W2o917DxOnM?=
 =?us-ascii?Q?yMdy2/eHajOPIUhhjzNcGZMBDH/g21+hnoeguvqctvGs8qtPd4dIcTyySMSh?=
 =?us-ascii?Q?8HT5EWw0XachGxfgrAMcOStunOxujxTJTMpr/GvjHPIyKwXVHQYA99/30T+2?=
 =?us-ascii?Q?+I5Ikn7kkqQbGTNwrrpOIYO6tNRMychC6foOdwqAKVPQZu7NrXuSOHTT+29T?=
 =?us-ascii?Q?CHSBvsAbZ08Otg6wWniBuFlk0oKrnlka4Xrr7YRBuCUkZG/qrzD8zNcZln19?=
 =?us-ascii?Q?jWRfK9a4yp7TqKKXbl3KS7ERtI0ak5daOPdmqw269cQac3agXbkWPTqRgCRm?=
 =?us-ascii?Q?vjiwIkZd1RSL91yuImi93d8/F8DlPJCVjxqLKo3Q2REV8E81oCi5uT7Ihj57?=
 =?us-ascii?Q?40/+WlE+DDa9Xf5ub8I8m0LWLXjcI8UJLL0M8b6NmKnCIuBhEYE/SHPMZqOi?=
 =?us-ascii?Q?1UQcxjpw8ZfA9qvaoyRDyugui5SYRER0N3PuOetiGTZ9K9FmGA08McokEHE7?=
 =?us-ascii?Q?IfVgYbZwzfklbxfKqVzwml/ivTom3HHPo0jrZqqTQMtbBQ7R2LA4TtwECmtj?=
 =?us-ascii?Q?fE3q92P409FSleCS26zj3Z9vZ+op8P/TAQPCw0FlPmAAqRUCqysjgOMd4FeB?=
 =?us-ascii?Q?QsRh4ydsT6vUFHck2wXwQBxGBrIPV4xM+E63r9P7GimvnAw1r2h7L7OX2ZTf?=
 =?us-ascii?Q?ozZpeCcjcLQMcnmxiYfNEJJLqPf/RArY0Df/o5dz3y/XNeWmGqpRu16bg4L9?=
 =?us-ascii?Q?rEKRcrtMrEqIhwwOxCEcgNijPXqfGMSF6JO7o9U2mcUt1hh/w3kQ6cF3wkLa?=
 =?us-ascii?Q?//1SjC+pknayMPNxGYT/HJxzVxTj2GSCWzCwWUHgcEmwmFPRjdWotG0a/I0l?=
 =?us-ascii?Q?V/VMLH35afVRPOE8bAJxLEIYgi63QqTBS2wz94S5pkOIJjRX+MNsCRPvDK3Z?=
 =?us-ascii?Q?UPawOEUUoZz+Q7FJ91DO7wpagHywOnLknsnyk9v189uKwagrIDW1gvI+0ptb?=
 =?us-ascii?Q?CdsETsdpqW998+o2XfhKhvg2grHH9eAvDM20RMdaOxCVoL9bQhhMZ2xtU0OR?=
 =?us-ascii?Q?XEOkeZz9W6OvOMTBHAHzi6CooZslQO9Yj/dI15fjxfcU4GmKklcgVgx5La1/?=
 =?us-ascii?Q?0+ofuB7Ag66CoFDvK6uaWEP8U84BxDkiSCPmKAkdTJtqkjPYXg7/GaPOKIzF?=
 =?us-ascii?Q?dIlXkZYKbx7Sw49y7xNUqWt6ypUyrRg8QmFSavHvkj8QfXUUE3xeHocQMFZY?=
 =?us-ascii?Q?+AD8B02GvbXHiwaYUs9Ckqjf6CL1EYjzI90z4HsAAGiUjiRPL5oWGI52S+Vv?=
 =?us-ascii?Q?lZyLd95C/+sCMwHkPXRTeTYcs5nLSyO7AvTt6bBGa0IdLkTBFPdhiV7HaLRm?=
 =?us-ascii?Q?mZcoAqgpFtQBiFHUttZm4xDK6AVGW89LuiA5650nw3EE/tiG5CwTVBHkfoTl?=
 =?us-ascii?Q?vD5HuZ8xf6hleLSBMH759LCDwMxVOI/gAnpW3D+ifeIf0VrCPhbJY1Z0sFNJ?=
 =?us-ascii?Q?S468PaacoFTdT8BskUsdl64f+9EVaf1IvhmFipSCrUYuEwox/VCgvWmAcaim?=
 =?us-ascii?Q?khPw/du/V0+Wa94roHK/icrGSOoao5LYQmrhh372J0iR85VH0kwZ7+21RbQ8?=
 =?us-ascii?Q?WwBZ3NmzMa5sK5G0AeGVWDISEn9xjGNj71quot8qhBRBg6FH9kaevXPpCGWC?=
 =?us-ascii?Q?AB94SgQZW3X2CX6VDW+EQrtZNy2Poy75QrWLCSqO1cZ/UPxtZLe5?=
Content-Type: text/plain; charset="us-ascii"
Content-ID: <2B52ABCBFD377E49964A4E19178B1658@namprd15.prod.outlook.com>
MIME-Version: 1.0
X-OriginatorOrg: fb.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: SA1PR15MB5109.namprd15.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 19d647ba-54c6-489e-efe5-08da39c4f9e1
X-MS-Exchange-CrossTenant-originalarrivaltime: 19 May 2022 18:25:39.4741
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 8ae927fe-1255-47a7-a2af-5f3a069daaa2
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: RkA5w1ln6KRxNp8wUe0F7eYeEep4LUU8zLrRPGs0H6+5OKW+ywDQ6JcCrLK237BfeGGNZ3o+KzkRy9//3sGjzg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BN8PR15MB3412
X-Proofpoint-GUID: YUcqvisxirbnHpZo1WNRm6pY47QDimqq
X-Proofpoint-ORIG-GUID: YUcqvisxirbnHpZo1WNRm6pY47QDimqq
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.205,Aquarius:18.0.874,Hydra:6.0.486,FMLib:17.11.64.514
 definitions=2022-05-19_06,2022-05-19_03,2022-02-23_01
X-Spam-Status: No, score=-3.3 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_LOW,
        RCVD_IN_MSPIKE_H3,RCVD_IN_MSPIKE_WL,SPF_HELO_NONE,SPF_NONE,
        T_SCC_BODY_TEXT_LINE 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


> On May 19, 2022, at 9:56 AM, Edgecombe, Rick P <rick.p.edgecombe@intel.com> wrote:
> 
> On Thu, 2022-05-19 at 06:42 +0000, Song Liu wrote:
>> Thinking more on this. Even huge page is not supported, we can
>> allocate
>> 2MB worth of 4kB pages and keep using it. This would help direct map
>> fragmentation. And the code would also be simpler. 
>> 
>> Rick, I guess this is inline with some of your ideas?
> 
> Yea, that is what I wondering. Potential benefits are just speculative
> though. There is a memory overhead cost, so it's not free.

Yeah, I had the same concern with memory overhead. The benefit should 
not exceed 0.2% (when 2MB page is supported), so I think we can use
4kB pages for now.  

> 
> As for the other question of whether to fix VM_FLUSH_RESET_PERMS. If
> there really is an intention to create a more general module_alloc()
> replacement soon, then I think it is ok to side step it. An optimal
> replacement might not need it and it could be removed in that case.
> Let's at least add a WARN about it not working with huge pages though.

IIUC, it will take some effort to let kernel modules use a solution 
like this. But there are some low hanging fruits, like ftrace and BPF
trampolines. Maybe that's enough to promote a more general solution. 

For the WARN, I guess we need something like this?

diff --git i/include/linux/vmalloc.h w/include/linux/vmalloc.h
index b159c2789961..5e0d0a60d9d5 100644
--- i/include/linux/vmalloc.h
+++ w/include/linux/vmalloc.h
@@ -238,6 +238,7 @@ static inline void set_vm_flush_reset_perms(void *addr)
 {
        struct vm_struct *vm = find_vm_area(addr);

+       WARN_ON_ONCE(is_vm_area_hugepages(addr));
        if (vm)
                vm->flags |= VM_FLUSH_RESET_PERMS;
 }


Thanks,
Song

> 
> I also think the benchmarking so far is not sufficient to make the case
> that huge page mappings help your workload since the direct map splits
> were also different between the tests. I was expecting it to help
> though. Others were the ones that asked for that, so just commenting my
> analysis here.