Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
From:   Song Liu <songliubraving@fb.com>
To:     Alexei Starovoitov <alexei.starovoitov@gmail.com>
CC:     Ilya Leoshkevich <iii@linux.ibm.com>, Song Liu <song@kernel.org>,
        bpf <bpf@vger.kernel.org>,
        Network Development <netdev@vger.kernel.org>,
        LKML <linux-kernel@vger.kernel.org>,
        Alexei Starovoitov <ast@kernel.org>,
        "Daniel Borkmann" <daniel@iogearbox.net>,
        Andrii Nakryiko <andrii@kernel.org>,
        "Kernel Team" <Kernel-team@fb.com>,
        Peter Zijlstra <peterz@infradead.org>, X86 ML <x86@kernel.org>
Subject: Re: [PATCH v6 bpf-next 6/7] bpf: introduce bpf_prog_pack allocator
Thread-Topic: [PATCH v6 bpf-next 6/7] bpf: introduce bpf_prog_pack allocator
Thread-Index: AQHYDwA7v2eTErzZP02KrqTA2Ue2qKxuJqkAgAAH3QCAAAUSgIAABaKAgAAC8wCAAAUaAIAAC8WAgAF+/gA=
Date:   Sun, 23 Jan 2022 01:03:25 +0000
Message-ID: <5E70BF53-E3FB-4F7A-B55D-199C54A8FDCA@fb.com>
References: <20220121194926.1970172-1-song@kernel.org>
 <20220121194926.1970172-7-song@kernel.org>
 <CAADnVQK6+gWTUDo2z1H6AE5_DtuBBetW+VTwwKz03tpVdfuoHA@mail.gmail.com>
 <7393B983-3295-4B14-9528-B7BD04A82709@fb.com>
 <CAADnVQJLHXaU7tUJN=EM-Nt28xtu4vw9+Ox_uQsjh-E-4VNKoA@mail.gmail.com>
 <5407DA0E-C0F8-4DA9-B407-3DE657301BB2@fb.com>
 <CAADnVQLOpgGG9qfR4EAgzrdMrfSg9ftCY=9psR46GeBWP7aDvQ@mail.gmail.com>
 <5F4DEFB2-5F5A-4703-B5E5-BBCE05CD3651@fb.com>
 <CAADnVQLXGu_eF8VT6NmxKVxOHmfx7C=mWmmWF8KmsjFXg6P5OA@mail.gmail.com>
In-Reply-To: <CAADnVQLXGu_eF8VT6NmxKVxOHmfx7C=mWmmWF8KmsjFXg6P5OA@mail.gmail.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?gLKQVzgUIZk8pxfCe0UNVeOXPCVVKDTfplUIkI9FkWum64Z5/3g/nq9GOx0p?=
 =?us-ascii?Q?7IKP7oXb1C1xjAX9ACoNf0T7TdmomdSWxHcEN4QdxpPkL4LfH/Cx+9vUkM+0?=
 =?us-ascii?Q?fUlJqa7lRp6XmwX1RLzh8/yeK6zgS/Oq21H0MjM9vmlZ9GCyXTSGGgpDYd0M?=
 =?us-ascii?Q?lVzmbyi+UEc2g6VIANrCdSZdrYX/NkY0LcpQZ8C+ewQ+NduE/hqmzqqCTAn3?=
 =?us-ascii?Q?6rXXh42N1YaNUQ/zUwDLO5iHdhn9QXeQ5MlLkpmsbIj0wlNhSDWe5goXIQsd?=
 =?us-ascii?Q?autK14IuqlVJ6F2GcxofiYOYIpnNnxZFNHycft/TItWGMbHETAK4yxnXl3Aj?=
 =?us-ascii?Q?FnbZl2FZ/ddHRydPFsHYN2c1hETF/Seq1C1YdaxgYbf6WmWP3yj6AKvCpRLy?=
 =?us-ascii?Q?W8Y9Cj736RnOpz/zoiBaF+Sh29gYd3U+Y7f/+10CD116oNlxmAzCk8aoGKwy?=
 =?us-ascii?Q?b78v2v0MIxh0JH94dHmavi7RWfwEMYh2Dk+csTXpsHaabrNOcVldvFgHgWPk?=
 =?us-ascii?Q?fZeOqtb8dJOFceKFpXHH74qRr0r0VZ/ltLoesCGssVpbL/aDXCdBwRmNBSNP?=
 =?us-ascii?Q?Mw4xeznUGBrsRcMlYxqck2f2gwQ98ImNM1wscWUVJs1JAHEZ6tWkUCDYo9aw?=
 =?us-ascii?Q?wD/p9FUNuJS1maOfD4G8hPBdkr+bnWdaY4F/XOITcX7bE2AxT8xeatI2Cten?=
 =?us-ascii?Q?k91zQMsed4eISz/dvpeFH7YcnFmW+1Fet4rJT8Ke7EeTSub8elC5ElAbA6AB?=
 =?us-ascii?Q?vth6lYivaHNeo9W+RqW5adVjz43jkWjjVlsZbcYHtvEV9vV/lw++IWybGGSr?=
 =?us-ascii?Q?yyAHDnks98yNRN9AheKdS2ZslzuGj0d5bh6Re/I4Q+wUWiUXlwffT2qhJGEh?=
 =?us-ascii?Q?Jzf5gO39ArnEz+C5AqyIGGc79hF4w4Zbjtwxqh/38Pm8UItwNKK79cbBK7td?=
 =?us-ascii?Q?kNIjSy9hNPjuUZKOjDDg1hiqgfTFkBdn+eBrGK3JN5U0WhgNMlFvZEoKbY5t?=
 =?us-ascii?Q?9wTA8p+MSmw+FjDU07S0FA+6s2VW6QXgTCF2oJH6hyj5F9PCCtK+gr5OhNjR?=
 =?us-ascii?Q?kLYZ9SrM5xaE8rhyPzPc6X+vHwbHMJ44AwztMnoDH+K/orwu15pX6DAzk6AR?=
 =?us-ascii?Q?qnTQMD33PTELkZeqIhABmgTI/33Mee9KoW/PZ7A/nPkBRl/tJz2vT7ynf31I?=
 =?us-ascii?Q?OTXZfKXu3WqpFQ23NwiaeMeQlJVAxmWFwjrC68a3Bcw3IdZO3dY4hdw1Hgf9?=
 =?us-ascii?Q?miR9X/F/gRct1SqJBQSgGLxlBOpPcoXwjbQ9CV1WWyyoawpgNM1izhbHgzzF?=
 =?us-ascii?Q?jZ7haVWIjN7Sih7H3W1qYF8SGQrbdM4qv9Vto4uIN4F2zFjmMcI6yPGL9yPN?=
 =?us-ascii?Q?KIjqBB9AnnQa4JmoUurVb8inUFAm0VwIvPKsAxYKk1ZCo4CHcdYOUCQlzXnm?=
 =?us-ascii?Q?9I0nhhBgu2wrylvYZvi69ov3CTz70hxOqwTDTwvXxWb5hnk931v8O3pbTqf5?=
 =?us-ascii?Q?7VNl3ooR5V0pRD4eojWLkpD07HblKgz3qkuWwpUI6r8Dh2kDIbuceAPgWmFg?=
 =?us-ascii?Q?6Jf67eDbRdoSd2Eej01We4eaG0bEUBPvFgfImeBD387DEk467idVNGRtGiYs?=
 =?us-ascii?Q?HHaSNmI+qB71QXPOl3CidWRwM6IKNkoNCE6r5mfs76A4CugnJ/NXpgFYtH9b?=
 =?us-ascii?Q?YXCYMA=3D=3D?=
Content-Type: text/plain; charset="us-ascii"
Content-ID: <5B7F2F9442F8164F90D7E633B99CED81@namprd15.prod.outlook.com>
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: SA1PR15MB5109.namprd15.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: b8933bc7-f15c-474b-ad43-08d9de0c2892
X-MS-Exchange-CrossTenant-originalarrivaltime: 23 Jan 2022 01:03:25.0842
 (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: tQn6c0yPqPity6NeSq8LJ7Z15IFNVE7rIqUzVVuCTev6X6RX0NMnnPr9aMm9pyz7i3Sr06PXmhOEfGkVn8r09g==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SJ0PR15MB4424
X-OriginatorOrg: fb.com
X-Proofpoint-ORIG-GUID: SsDQVoNF9OEg6m3gk0CYTsndYApQ23kt
X-Proofpoint-GUID: SsDQVoNF9OEg6m3gk0CYTsndYApQ23kt
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.205,Aquarius:18.0.816,Hydra:6.0.425,FMLib:17.11.62.513
 definitions=2022-01-22_10,2022-01-21_01,2021-12-02_01
X-Proofpoint-Spam-Details: rule=fb_outbound_notspam policy=fb_outbound score=0 suspectscore=0
 bulkscore=0 mlxlogscore=999 spamscore=0 mlxscore=0 phishscore=0
 adultscore=0 impostorscore=0 malwarescore=0 lowpriorityscore=0
 clxscore=1015 priorityscore=1501 classifier=spam adjust=0 reason=mlx
 scancount=1 engine=8.12.0-2201110000 definitions=main-2201230005
X-FB-Internal: deliver
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


> On Jan 21, 2022, at 6:12 PM, Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote:
> 
> On Fri, Jan 21, 2022 at 5:30 PM Song Liu <songliubraving@fb.com> wrote:
>> 
>> 
>> 
>>> On Jan 21, 2022, at 5:12 PM, Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote:
>>> 
>>> On Fri, Jan 21, 2022 at 5:01 PM Song Liu <songliubraving@fb.com> wrote:
>>>> 
>>>> In this way, we need to allocate rw_image here, and free it in
>>>> bpf_jit_comp.c. This feels a little weird to me, but I guess that
>>>> is still the cleanest solution for now.
>>> 
>>> You mean inside bpf_jit_binary_alloc?
>>> That won't be arch independent.
>>> It needs to be split into generic piece that stays in core.c
>>> and callbacks like bpf_jit_fill_hole_t
>>> or into multiple helpers with prep in-between.
>>> Don't worry if all archs need to be touched.
>> 
>> How about we introduce callback bpf_jit_set_header_size_t? Then we
>> can split x86's jit_fill_hole() into two functions, one to fill the
>> hole, the other to set size. The rest of the logic gonna stay the same.
>> 
>> Archs that do not use bpf_prog_pack won't need bpf_jit_set_header_size_t.
> 
> That's not any better.
> 
> Currently the choice of bpf_jit_binary_alloc_pack vs bpf_jit_binary_alloc
> leaks into arch bits and bpf_prog_pack_max_size() doesn't
> really make it generic.
> 
> Ideally all archs continue to use bpf_jit_binary_alloc()
> and magic happens in a generic code.
> If not then please remove bpf_prog_pack_max_size(),
> since it doesn't provide much value and pick
> bpf_jit_binary_alloc_pack() signature to fit x86 jit better.
> It wouldn't need bpf_jit_fill_hole_t callback at all.
> Please think it through so we don't need to redesign it
> when another arch will decide to use huge pages for bpf progs.
> 
> cc-ing Ilya for ideas on how that would fit s390.

I guess we have a few different questions here:

1. Can we use bpf_jit_binary_alloc() for both regular page and shared 
huge page? 

I think the answer is no, as bpf_jit_binary_alloc() allocates a rw 
buffer, and arch calls bpf_jit_binary_lock_ro after JITing. The new 
allocator will return a slice of a shared huge page, which is locked
RO before JITing. 

2. The problem with bpf_prog_pack_max_size() limitation. 

I think this is the worst part of current version of bpf_prog_pack, 
but it shouldn't be too hard to fix. I will remove this limitation 
in the next version. 

3. How to set proper header->size? 

I guess we can introduce something similar to bpf_arch_text_poke() 
for this? 


My proposal for the next version is:
1. No changes to archs that do not use huge page, just keep using 
   bpf_jit_binary_alloc.

2. For x86_64 (and other arch that would support bpf program on huge
   pages):
   2.1 arch/bpf_jit_comp calls bpf_jit_binary_alloc_pack() to allocate
       an RO bpf_binary_header;
   2.2 arch allocates a temporary buffer for JIT. Once JIT is done, 
       use text_poke_copy to copy the code to the RO bpf_binary_header. 

3. Remove bpf_prog_pack_max_size limitation. 


Does this sound reasonable?

Thanks,
Song