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:     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 v5 bpf-next 7/7] bpf, x86_64: use bpf_prog_pack allocator
Thread-Topic: [PATCH v5 bpf-next 7/7] bpf, x86_64: use bpf_prog_pack allocator
Thread-Index: AQHYDjH7IlFLQbwkPUWXKcNPE2HSyKxs6t2AgADYTACAAAo4gIAAB3cA
Date:   Fri, 21 Jan 2022 18:56:39 +0000
Message-ID: <ACCB0C73-9968-4DB8-9B8D-97560B32D8DD@fb.com>
References: <20220120191306.1801459-1-song@kernel.org>
 <20220120191306.1801459-8-song@kernel.org>
 <CAADnVQL-TAZD6BbN-sXDpAs0OHFWGg3e=RafBQ10=ExXESNQgg@mail.gmail.com>
 <F72EEF0D-4F61-4AE1-B2A1-D16A5DBCCC37@fb.com>
 <CAADnVQLWj6cVE=OEqNfSBcAzFJDHPF8sPfqGfaCknV+Q=1HOmQ@mail.gmail.com>
In-Reply-To: <CAADnVQLWj6cVE=OEqNfSBcAzFJDHPF8sPfqGfaCknV+Q=1HOmQ@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?Tmtd5pRAXuqe38YzRzHa4Uv44MeH858ZMl0NfxutxiUIKOlASTBLmj+shXkS?=
 =?us-ascii?Q?lwe/P+vUNu6O/5CnbIKY5ASLGrTmYc77jVKox5pk75ehlvKQHBm46mcIsQfU?=
 =?us-ascii?Q?Gw8844gHCyYOwUVHlPVapRlavo3zcB0iFZZsKGX+rJsHMYI7wVEi/denQ84y?=
 =?us-ascii?Q?ujkcs0vuhlE7dzNxpurUHuU88frNlqZa/bNiPtxvbjcfjTiaiBpQgferxtUY?=
 =?us-ascii?Q?M4mI2SP5rV/+AfWvNQoqQeTaPuWaBEPhqrN/9QYwtbbg6wJz5+FVatxqjw5L?=
 =?us-ascii?Q?7iYlqLygak3tSFtre15yHvqmJVjPwQY67IKgkZWp4TUWI+eZqLoSVxmxsxjc?=
 =?us-ascii?Q?rxAbjhyHQjgw/2GkH3Yg6tgCzDwVtVlBt5P/HGbE5M2T/pw+7Ul89dyxzqDT?=
 =?us-ascii?Q?E4f2sxUV5W/5YDQErGXC24GU03ArIgnkkdRWgdLCmgUVZQ8ncKBDdQ0BqFfe?=
 =?us-ascii?Q?Prma4qDm9qYR8kYESWO9CWQhHgACbZZQVWXBusJ0sTh531xnHlGnzOJJELII?=
 =?us-ascii?Q?k6hibo7xlg2pEjkIdE8lpxsp46pU/pRY1YwlfUNhJMPvqQGLPEBaKcWdOup4?=
 =?us-ascii?Q?MEI7VSd0lVa3sucRpovGoBOCDFxyPDaPxRzLR3nmqqKUsUwuVsr2zVI7fkQ5?=
 =?us-ascii?Q?3D5LmCGYh/3DCZI6wIb8uk88TPZ37FIKUVn6GPlgYkCj5bTgc6G85JXyxaN8?=
 =?us-ascii?Q?l1r5EjCqPfElFf2g91FoUv/+pdcnoIRF8jZYm9xWmADUaUyxw70JLQymaOdR?=
 =?us-ascii?Q?AH6rgBrmQrRH9GYBFOiv09+pBcRqG70hieFdEs9M5RZWOSKsxQxwKkUfhwc8?=
 =?us-ascii?Q?bIcoHqDog2PLks5coL1CE6kKIq1/sqlxZEuylisAj+SGT9TGPPh8j4o0IS6Y?=
 =?us-ascii?Q?oO4B+Vehis24Dt7jjttQrAho6yIoA2iUMSLYYVUZzPIl6xJYvGkCXfg+tLdG?=
 =?us-ascii?Q?5bVL8IBYk6RxFzVxuufyYXRhZgtNOAkwjRJs9pfnroh7FyJ4CPkPxEZKWZ0M?=
 =?us-ascii?Q?Nu7gqKSCvEkqpZQg0RcHWBqRXC3z9lKWKSyDDQPd2ibs1E1dg21GEBSN8GVl?=
 =?us-ascii?Q?pArDFia9Jg6ZtbmdTR9KVFducDjtEikTF0ffw2TSOXjESpLcnixe3R+wqNZx?=
 =?us-ascii?Q?AWkNrznjnKDqZJ/Xtx6SfRHDmh6IjPlP5tIFfpJ2arEg0uR86eJqrjiQeRsN?=
 =?us-ascii?Q?+akpiqb5mdjRpFPZF5NKz1/5uGSXvuz7J4BI+oOPJRAx48ghdtQcRBP8MTDp?=
 =?us-ascii?Q?TlAx8gFVSIrictCjs6CEojbIrS+NmefTHdvJDAqfUOl3bjCTDE+LzMbb1atC?=
 =?us-ascii?Q?Wmxy+Y9Jdk/XBlD86tDhCvoL/PPyf8r8c8He11M9VRtI3PsMbA4uUaKSvlId?=
 =?us-ascii?Q?0znwHbCHWJ4CmsG30dwf7i6ll65ubCP8IWufrWQxBNzNq4KfHNtyKSaQYNfw?=
 =?us-ascii?Q?H5wwWV58mbKb654fn9sYjXz1/1ZogPKmvssRMrjKDJYKMLjWapyWS10P9O31?=
 =?us-ascii?Q?ogWTC+fRWzf04t/hsBN2uRMs7TtTTa39mzWuqD+XYpaVAk6LcxrCNiDT6Zf2?=
 =?us-ascii?Q?mqI++pJxEY0NHBOAJJnFVun/bqgAof0byVvHPF1oMiTmflwIiX/OcPS7J/NN?=
 =?us-ascii?Q?Qu7+dcmwL9xHoCdUqQA14KVF/Em/7YZm/qe0YgF6HGA2LffhNRXe4blI17Bz?=
 =?us-ascii?Q?OAU7aw=3D=3D?=
Content-Type: text/plain; charset="us-ascii"
Content-ID: <F994C7ED32154B4799DAEC7B26F15B7F@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: 7ed65530-8ee7-46cb-a0af-08d9dd0fc1b3
X-MS-Exchange-CrossTenant-originalarrivaltime: 21 Jan 2022 18:56:39.2408
 (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: 6pxyMJuCnwvGqU1aA9Vzwfr8g699fDFv9yNomczWKpfsjj9iey6qdis4tC2ks8wKtwoyhuRGUPnLQ3k04FS91Q==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BN8PR15MB3329
X-OriginatorOrg: fb.com
X-Proofpoint-ORIG-GUID: 2Zmm6rUg6QsdE6NDjrZ5Ys-77BZm3ugT
X-Proofpoint-GUID: 2Zmm6rUg6QsdE6NDjrZ5Ys-77BZm3ugT
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-21_09,2022-01-21_01,2021-12-02_01
X-Proofpoint-Spam-Details: rule=fb_outbound_notspam policy=fb_outbound score=0 impostorscore=0
 spamscore=0 malwarescore=0 adultscore=0 clxscore=1015 mlxlogscore=751
 bulkscore=0 phishscore=0 suspectscore=0 lowpriorityscore=0 mlxscore=0
 priorityscore=1501 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2201110000 definitions=main-2201210123
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 10:29 AM, Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote:
> 
> On Fri, Jan 21, 2022 at 9:53 AM Song Liu <songliubraving@fb.com> wrote:
>>> 
>>> the header->size could be just below 2MB.
>>> I don't think kzalloc() can handle that.
>> 
>> Technically, kzalloc can handle 2MB allocation via:
>>  kzalloc() => kmalloc() => kmalloc_large() => kmalloc_order()
>> 
>> But this would fail when the memory is fragmented. I guess we should use
>> kvmalloc() instead?
> 
> Contiguous 2MB allocation?

Yeah, I tried that both kzalloc() and kvmalloc() could get 2MB memory.
I think kzalloc will fail when the memory is fragmented, but I haven't 
confirmed it yet.

> 
>>> 
>>>> +                               if (!tmp_header) {
>>>> +                                       bpf_jit_binary_free_pack(header);
>>>> +                                       header = NULL;
>>>> +                                       prog = orig_prog;
>>>> +                                       goto out_addrs;
>>>> +                               }
>>>> +                               tmp_header->size = header->size;
>>>> +                               tmp_image = (void *)tmp_header + ((void *)image - (void *)header);
>>> 
>>> Why is 'tmp_image' needed at all?
>>> The above math can be done where necessary.
>> 
>> We pass both image and tmp_image to do_jit(), as it needs both of them.
>> I think maintaining a tmp_image variable makes the logic cleaner. We can
>> remove it from x64_jit_data, I guess.
> 
> I'd remove from x64_jit_data. The recompute is cheap.

Will do. 

> 
> Speaking of tmp_header name... would be great to come up
> with something more descriptive.
> Here both tmp_header/tmp_image and header/image are used at the same time.
> My initial confusion with the patch was due to the name 'tmp'.
> The "tmp" prefix implies that the tmp_image will be used first
> and then it will become an image.
> But it's not the case.
> Maybe call it 'rw_header' and add a comment that 'header/image'
> are not writeable directly ?
> Or call it 'poke_header' ?
> Other ideas?

I think rw_header/rw_image is good. poke_header is confusing, as we will
text_poke "header". 

Thanks,
Song