Message-ID: <dd811304-44ed-0372-8fe7-00c425a453dd@intel.com>
Date:   Fri, 3 Mar 2023 12:22:21 +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>
From:   Alexander Lobakin <aleksander.lobakin@intel.com>
In-Reply-To: <36d42e20-b33f-5442-0db7-e9f5ef9d0941@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?QVVXM250VVlsY2UxRzRLVTNDQk4xUW1pcEpFMWZkN3BUdkpDdDdVMHBrSGdL?=
 =?utf-8?B?RlFzY1hSWUwwY2FKN3VQTThUWTJJT0h0SnlkaFE0SzY3RHNZUjNiNy83MHpz?=
 =?utf-8?B?MVVieFdQNisyenRqUTdEbXlIWllUNlRVRkUxWHBXUkNrVmZhUkh3aExrekN1?=
 =?utf-8?B?VzhqQ0t6NGVja0VkWFNzVVo3SFJjaEFkYkV5UWNkUmpwWVRpWW0rajNwUmxP?=
 =?utf-8?B?YkdidDROZnRUYlRQTTRYbHB3ZUNoUTdSRHVuWUtocXFobW9IQVozY2FqL0ZN?=
 =?utf-8?B?Mk14cFJRS3ZUWFZZQ2tGNi9DVU00L1gzSjdQNUc3Wlg4eFpJUXVaV3M1RUM5?=
 =?utf-8?B?Q204eDgyVGNnZm9MR3NWL3ArNHltWFVKZFRNaE5VZnhMUDdNYnlGaWtnbmxG?=
 =?utf-8?B?WHc2K3VsTFJnbHBmazZOTmprSUJTTDVsbXJ0Yys1R2tsMEJiZDVteXRtWFUz?=
 =?utf-8?B?Y0hNSklaNm8rUmZDcEJUUmFRSEtPeTJCZzRqUjl6Tk5oOEU1NXZCQlJmWmI4?=
 =?utf-8?B?V2Z3bVIxMGc1NmRuQXZqWWhTNUQ4YU1RWVZGaTcxTnB0REpBVVZLSy9VeXNX?=
 =?utf-8?B?eDcybG9KTGlrSWxwSmNXSjZwcEJpblNkTjUxZWdlUjZwTS85Z1dHdWE5T1JN?=
 =?utf-8?B?ZU5MNS9hWjVLTlErTERTK01lZUxjY2pVQTVWV3RtL0NBT3ZmU1Y5dTRrOUtv?=
 =?utf-8?B?UDl1YWJBU2V5aDZsWms4cVlvTXl0Y1pNNzc2bklQTHdaWlo5Y3BwMTMyZGRl?=
 =?utf-8?B?bkVCLy9sSytHc3dvaHpxSFVlVGZoQWdMR3k1ckRWYW5IQ0hrdkxFQzc2SVJV?=
 =?utf-8?B?akdEZ2dnK0Nsb0wyeEtsNitKcjdoK1ZlZWROa2NwTENXSVBiWUl3eFA3WEth?=
 =?utf-8?B?YjRPamtubTlmVVM0MlF5NHFHL2pBcFc0ZTRYOHlRUTJMZnRqeWFncDVzd28x?=
 =?utf-8?B?VGw5bUs2QTBmZWpKVFVQVk95K1dtTmYzUDVHbnpzY0l4Wk43RWlFVTVPS3RX?=
 =?utf-8?B?YXI5djkwMU1Jak1TMkIzOHlsd3NSNUF4QjIzaFJ3RW9ZQzNCRnBvNnVVcHRJ?=
 =?utf-8?B?cmNhaXJCL1c5OG5ZMDlEejBMUVRWMlpybks2ZC9WeHNzbGpsWU5CVGw1Z3pn?=
 =?utf-8?B?VVE1U2pkZTlXdDJKWFpDUnBnekVCTHdGbmR4R1ltaXdOMUdpc0J5NlVaV2hP?=
 =?utf-8?B?eUdzaDU3bmZkaGdqNlVTTEQ2S2NrTVBrdGl2U1E1aU1LWXNxa0ZuNDNOdGNk?=
 =?utf-8?B?RDJmN1Nzc2N3YjhRbS90NUF1M1dBeGJZaXZ5bnp2SzNzVU9pMVM5a3FsRDdy?=
 =?utf-8?B?M0ZwSHRBMFc3QUxXajBUUTVsWTFBNDRVaWltWWZ3eEFaYzdHZmFPd3B6K3hT?=
 =?utf-8?B?L2s5UDN1QlZJS2dQZmlqTWFNY0pRTFVoT09Yc1lRWnBPYjVDbTRPc3l6V1FG?=
 =?utf-8?B?eG01bUxqVHlKWE5saVNzRndYNHFJUTIzZzZHbEZ6Y1B6UDh3cExUQ05xelBM?=
 =?utf-8?B?bTZLL3ZZaUpmSHdHNUFIY1B5YWEycEtRK0dHVEMrSkZMYnhzK01CdnNnTWVa?=
 =?utf-8?B?T1JZdlNlWTJqVGtLSEpXcnFJbTd6UTVTYnd0ZWJuTVlvQlhJRitPT3ZVQmtL?=
 =?utf-8?B?MElVWEJpRXZoZmZLamwxTDMycnJnSkZxZmFBcE42RTJlRkxRYUxmZW45dHJr?=
 =?utf-8?B?eFJsVGoyd1plSnhrTjAwYXVGWis0VEZuY2o4TUxCVW1FdzN5dE52eHdvNHpB?=
 =?utf-8?B?bEg3a2FtZ29DbVc5VDBlRXNLUTlvL3ZFeUR5ajliNkJQWW1hSCtrNFRVeVc4?=
 =?utf-8?B?aUdSREhmWlAvV1JNaS9iUkhXTE40TWt2N0dYbFhsWVVib0hud1JNMGZ3VWo1?=
 =?utf-8?B?OFQ4N3BOcWpXNFcyWldyR1lneUtxbG9YQ1Jzc1VNRjlCbVVGY2ZXV2YzdW5p?=
 =?utf-8?B?Tlc2NFNHMnJkQk1ndTNuVE5CZ0RwalgzY29oZGtISW9WNEg1OWxxQisvaStt?=
 =?utf-8?B?Q2xCeTc0VkdsODZNOWNvVzdnWXVVVGx1T3c0WlFOUDRhNVMrdlBBWEVFRFhy?=
 =?utf-8?B?VVI2d1YyU1Z2RnBaWUlSSlpHeTJHR09XRjVyVmhRaGlYQ0padGl1am42cUxT?=
 =?utf-8?B?czd0T0hQdlkvZStmblUyeVl3UXlaM290KzlGbWcydG51a05XQ0U4cUtIbkpa?=
 =?utf-8?Q?JqXK22CcUUesH9R09fpaVh0=3D?=
X-MS-Exchange-CrossTenant-Network-Message-Id: afd89996-ea5c-480e-f22b-08db1bd9afa5
X-MS-Exchange-CrossTenant-AuthSource: DM6PR11MB3625.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 03 Mar 2023 11:23:17.3829
 (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: OBFeS/dLQBYgwlfTk3eJEWPI3F14K1vby7V4E3gycnXI5RLqQx8r45q6ZJyk+oXSzfN4XJ/x2gDXen+Y1sLJEP6CWX9S1LsPhVdp2FkK7cg=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MW3PR11MB4762
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: Thu, 2 Mar 2023 10:30:13 +0800

> On 2023/3/2 0:03, Alexander Lobakin wrote:
>> __xdp_build_skb_from_frame() state(d):
>>
>> /* Until page_pool get SKB return path, release DMA here */
>>
>> Page Pool got skb pages recycling in April 2021, but missed this
>> function.

[...]

> We both rely on both skb->pp_recycle and page->pp_magic to decide
> the page is really from page pool. So there was a few corner case
> problem when we are sharing a page for different skb in the driver
> level or calling skb_clone() or skb_try_coalesce().
> see:
> https://github.com/torvalds/linux/commit/2cc3aeb5ecccec0d266813172fcd82b4b5fa5803
> https://lore.kernel.org/netdev/MW5PR15MB51214C0513DB08A3607FBC1FBDE19@MW5PR15MB5121.namprd15.prod.outlook.com/t/
> https://lore.kernel.org/netdev/167475990764.1934330.11960904198087757911.stgit@localhost.localdomain/

And they are fixed :D
No drivers currently which use Page Pool mix PP pages with non-PP. And
it's impossible to trigger try_coalesce() or so at least on cpumap path
since we're only creating skbs at that moment, they don't come from
anywhere else.

> 
> As the 'struct xdp_frame' also use 'struct skb_shared_info' which is
> sharable, see xdp_get_shared_info_from_frame().
> 
> For now xdpf_clone() does not seems to handling frag page yet,
> so it should be fine for now.

xdpf_clone() clones a frame to a new full page and doesn't copy its
skb_shared_info.

> 
> IMHO we should find a way to use per-page marker, instead of both
> per-skb and per-page markers, in order to avoid the above problem
> for xdp if xdp has a similar processing as skb, as suggested by Eric.
> 
> https://lore.kernel.org/netdev/CANn89iKgZU4Q+THXupzZi4hETuKuCOvOB=iHpp5JzQTNv_Fg_A@mail.gmail.com/

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.

> 
>>  
>>  	/* Allow SKB to reuse area used by xdp_frame */
>>  	xdp_scrub_frame(xdpf);
>>

Thanks,
Olek