Message-ID: <0bc28bea-78f5-bcce-2d45-e6f6d1a7ed40@intel.com>
Date:   Mon, 6 Mar 2023 12:58:49 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.8.0
Subject: Re: [PATCH bpf-next v1 1/2] xdp: recycle Page Pool backed skbs built
 from XDP frames
Content-Language: en-US
To:     Yunsheng Lin <linyunsheng@huawei.com>
CC:     Alexei Starovoitov <ast@kernel.org>,
        Daniel Borkmann <daniel@iogearbox.net>,
        Andrii Nakryiko <andrii@kernel.org>,
        Martin KaFai Lau <martin.lau@linux.dev>,
        Maciej Fijalkowski <maciej.fijalkowski@intel.com>,
        Larysa Zaremba <larysa.zaremba@intel.com>,
        =?UTF-8?Q?Toke_H=c3=b8iland-J=c3=b8rgensen?= <toke@redhat.com>,
        Song Liu <song@kernel.org>,
        Jesper Dangaard Brouer <hawk@kernel.org>,
        Jakub Kicinski <kuba@kernel.org>, <bpf@vger.kernel.org>,
        <netdev@vger.kernel.org>, <linux-kernel@vger.kernel.org>
References: <20230301160315.1022488-1-aleksander.lobakin@intel.com>
 <20230301160315.1022488-2-aleksander.lobakin@intel.com>
 <36d42e20-b33f-5442-0db7-e9f5ef9d0941@huawei.com>
 <dd811304-44ed-0372-8fe7-00c425a453dd@intel.com>
 <7ffbcac4-f4f2-5579-fd55-35813fbd792c@huawei.com>
 <9b5b88da-0d2d-d3f3-6ee1-7e4afc2e329a@intel.com>
 <98aa093a-e772-8882-b0e3-5895fd747e59@huawei.com>
From:   Alexander Lobakin <aleksander.lobakin@intel.com>
In-Reply-To: <98aa093a-e772-8882-b0e3-5895fd747e59@huawei.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?NUxDUk5GTitmaVRFbXJFaFlzc2J3cnMxUkpqSjhPSm1Yd1NDSjhra0RPdkZP?=
 =?utf-8?B?RjhQMGw3d3lKd3lwWS9hUmdwcm1kZk9jS3NEa0hHVGVBOHZXL3RPMUtPdXRl?=
 =?utf-8?B?YzdpaVhNdktrSUpZOWpZNnhoUnB2WUd5OE1ic1BNUGRmWVlBcytGNXdGTWNO?=
 =?utf-8?B?a05DRU1UYVFQOXBaUEJGS0MyZ3Zha0lnYk40SFVkTzRKSnA3K250SXhDTDZM?=
 =?utf-8?B?V1NtZHNLSEdmRWFmK2JTdHRsS0MxTWRaVGJ0QVVjRzlqWkN6eUpFaldHN255?=
 =?utf-8?B?MHN3dk4zNVU5cE1mVGVvQk5UVDk3Yi9WM1Y5TzRrektHN0lVZmd6SS9aQTNU?=
 =?utf-8?B?QVBYa0tDSDM3WVF2eE5ob3RlSGl3MXhDemdvM2Y3bHpnL1E0L3hwd1lVYWFq?=
 =?utf-8?B?QklPUHBKYkY2NFJ4M1BYSTBlTGtDUE5wdUFKbEIvSVZlSDY0WVMrZW8yR21N?=
 =?utf-8?B?aDZGVnBFTVVuZWNxd29RRzNKR1dCY1RWSkJORzRTMEJIZ0IwVTNGUnNQczBJ?=
 =?utf-8?B?OGZaaDlqWUllck1JYUxQSGIzbXdlTkxYM3dXMHF1MEtYOFVhQWtHMGV5SjVy?=
 =?utf-8?B?YVp5RnJsN3RvR21YNG1VaFJXTU5RcDVKTGpvVU5FY3U2amdpbk9yTGFSVVpY?=
 =?utf-8?B?d0JOWTFIVWZybGNWSVdNM295VEtQS29QRFRxbnEwQnU1bldLeVdJQkYxS3RM?=
 =?utf-8?B?ZEpGWSt2d3ZnL2pNS0h4Yll4Z2F3YVN5Rm1nT0pvelowZEp3M0E5QkZhcFkr?=
 =?utf-8?B?RWx0eXRhN3pVRnprNUZpMm1LUnArc3h5Ky9KSTRsYVo3Y3dsZXFRQXFUTDJC?=
 =?utf-8?B?LzNObEEvYXI4aHFoK3lQMEd3NXk4RnMwWEdlTUJLcGlXMEtnOEMyQWlmb2Va?=
 =?utf-8?B?a0kvdVppd3lJczlObVNQQ0VwZENGcUhIbjNWNDI2SHVPL0pLbVg4WmZ3dWkw?=
 =?utf-8?B?K0w4TkR0SWt3N0RZbFE2V01SRXNqK1pwM3o2NVdiNHBnUXRkc3BYaTczMlU5?=
 =?utf-8?B?RkJ2cnNUd2d1Y0x0ZnhtL0RiM1ZkNU5aanZteFV4cnp1VXdGUktSM3lwME5B?=
 =?utf-8?B?a25IYTdQdXRKRG9mN0ZNWHZuemV3Y1ZodFFzdWFNM1dSeGRoa0tDRkprekRo?=
 =?utf-8?B?V0Rrek5zVDBNM3pESG1UT0xub1M2TGxlL05mTFVJRmlEdkhyZlcwMDJiQUNL?=
 =?utf-8?B?S0x5Q1E0RTdOcjAvVXBtVTNmVXROUE9vek8xWVR5WXN5WVZ1bGNtN3J0aVls?=
 =?utf-8?B?ZnNRdmo1czBGUW5zeTVuU3MyMHdqNmpNdTdMVENMMnZwczl0L0hKUXI2a2Rv?=
 =?utf-8?B?QmxXODdSenJUU29vUGJrdkx3YVZITFdva3QwbkFlb1UxdGxMOFBZSUwzeVNH?=
 =?utf-8?B?SVVxSmFoQ1MxSWcvTCtjcy9NRDZUUCtNYUtrVTdvQzA0YjlDRldzMFdrM3Fl?=
 =?utf-8?B?aEJiWWhWOHBkZ0NDRnNQY1VLbDFtaVdMWkppTXlkYmtWZWZaUFppaDBTY1Zl?=
 =?utf-8?B?T1hWSkN0RE9wcmt6UStiNkwzdDZnazQ1SVBtV3BDdVoyUVpMQ2h4Z3ExbTEv?=
 =?utf-8?B?R0xYZ01XejdNTlU1eDBLVndMc1hSanVscUt0YnRDSXp3cldGTUpSNlpoRWJz?=
 =?utf-8?B?WVpQSHpEMTl2WFdKTXdtNjVFOUMxRy9qeUxOZFZORTV0UUoyZWZkZVI4cVVM?=
 =?utf-8?B?aVlKOURZcXROZ2ozRVdiemlnVnZodGUwMlY3ZmVzWFNta3RiWVYrLzM2UzFB?=
 =?utf-8?B?cTVCcEJYREhSQnFjbWp3TjI1eU80VlEvNGcza0gzOFNoVDc4dkpxV0lrVzVi?=
 =?utf-8?B?WnVvaEk0ZWxUNkF0TmVhRjhGaXlOVVRXTUFPYmdvYy9tMnNQZ1BMV2pHOHA2?=
 =?utf-8?B?YUt5aCtxaXlRRytyMUZkQmhzNWZQQ1VlL2VYQzZONXpMdVVQQjNwVy9lbmNw?=
 =?utf-8?B?NGcrdEx4ZTZuQ3JJbFNVaDgyemwzZzVvUFMvR3p3RkJKU25reWZQN3ZwZE9K?=
 =?utf-8?B?WHd6dk1JMGJza1FzM2JQS1UvNjByaEFhTEVOcHpmV3VHVmg2MVhFR1FtS0Iz?=
 =?utf-8?B?U3hNT1NjazAvZWxST01iWjdyb2VJT0txVEY3Zi9NMERBazdHR2pGMzBKeG9r?=
 =?utf-8?B?SmdpbmNjcm1ldUd0MXE0OVVITHVJMEt2NUNoRnNBcXB4QUxBaXVNRkZDSTFO?=
 =?utf-8?Q?PwjNf9kRNVue4yyGxq+aGjA=3D?=
X-MS-Exchange-CrossTenant-Network-Message-Id: bb016fa1-7336-4270-d2e1-08db1e3a59c5
X-MS-Exchange-CrossTenant-AuthSource: DM6PR11MB3625.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 06 Mar 2023 12:00:16.8630
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: PMbO52cbVNSqNUevCgi/lhT24onFpPVaKCaPGFBvVG9PDi215ufD3Hcv4OT8JGs0tpaaICwlIwj88a2duoVfHV8DMzOq7QwFeZ2o92WXjKc=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH0PR11MB4775
X-OriginatorOrg: intel.com
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

From: Yunsheng Lin <linyunsheng@huawei.com>
Date: Mon, 6 Mar 2023 09:09:31 +0800

> On 2023/3/3 21:26, Alexander Lobakin wrote:
>> From: Yunsheng Lin <linyunsheng@huawei.com>
>> Date: Fri, 3 Mar 2023 20:44:24 +0800
>>
>>> On 2023/3/3 19:22, Alexander Lobakin wrote:
>>>> From: Yunsheng Lin <linyunsheng@huawei.com>
>>>> Date: Thu, 2 Mar 2023 10:30:13 +0800
>>
>> [...]
>>
>>>> And they are fixed :D
>>>> No drivers currently which use Page Pool mix PP pages with non-PP. And
>>>
>>> The wireless adapter which use Page Pool *does* mix PP pages with
>>> non-PP, see below discussion:
>>>
>>> https://lore.kernel.org/netdev/156f3e120bd0757133cb6bc11b76889637b5e0a6.camel@gmail.com/
>>
>> Ah right, I remember that (also was fixed).
>> Not that I think it is correct to mix them -- for my PoV, a driver
>> shoule either give *all* its Rx buffers as PP-backed or not use PP at all.
>>
>> [...]
>>
>>>> As Jesper already pointed out, not having a quick way to check whether
>>>> we have to check ::pp_magic at all can decrease performance. So it's
>>>> rather a shortcut.
>>>
>>> When we are freeing a page by updating the _refcount, I think
>>> we are already touching the cache of ::pp_magic.
>>
>> But no page freeing happens before checking for skb->pp_recycle, neither
>> in skb_pp_recycle() (skb_free_head() etc.)[0] nor in skb_frag_unref()[1].
> 
> If we move to per page marker, we probably do not need checking
> skb->pp_recycle.
> 
> Note both page_pool_return_skb_page() and skb_free_frag() can
> reuse the cache line triggered by per page marker checking if
> the per page marker is in the 'struct page'.

Ah, from that perspective. Yes, you're probably right, but would need to
be tested anyway. I don't see any open problems with the PP recycling
right now on the lists, but someone may try to change it one day.
Anyway, this flag is only to do a quick test. We do have
sk_buff::pfmemalloc, but this flag doesn't mean every page from this skb
was pfmemalloced.

> 
>>
>>>
>>> Anyway, I am not sure checking ::pp_magic is correct when a
>>> page will be passing between different subsystem and back to
>>> the network stack eventually, checking ::pp_magic may not be
>>> correct if this happens.
>>>
>>> Another way is to use the bottom two bits in bv_page, see:
>>> https://www.spinics.net/lists/netdev/msg874099.html
>>>
>>>>
>>>>>
>>>>>>  
>>>>>>  	/* Allow SKB to reuse area used by xdp_frame */
>>>>>>  	xdp_scrub_frame(xdpf);
>>>>>>
>>>>
>>>> Thanks,
>>>> Olek
>>>> .
>>>>
>>
>> [0] https://elixir.bootlin.com/linux/latest/source/net/core/skbuff.c#L808
>> [1]
>> https://elixir.bootlin.com/linux/latest/source/include/linux/skbuff.h#L3385
>>
>> Thanks,
>> Olek
>> .
>>

Thanks,
Olek