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;
Message-ID: <413e3e21-e941-46d0-bc36-fd9715a55fc4@intel.com>
Date:   Fri, 30 Jun 2023 17:34:02 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.12.0
Subject: Re: [PATCH RFC net-next 2/4] net: page_pool: avoid calling no-op
 externals when possible
Content-Language: en-US
To:     Alexander Duyck <alexander.duyck@gmail.com>
CC:     "David S. Miller" <davem@davemloft.net>,
        Eric Dumazet <edumazet@google.com>,
        Jakub Kicinski <kuba@kernel.org>,
        Paolo Abeni <pabeni@redhat.com>,
        Maciej Fijalkowski <maciej.fijalkowski@intel.com>,
        Larysa Zaremba <larysa.zaremba@intel.com>,
        Yunsheng Lin <linyunsheng@huawei.com>,
        Alexander Duyck <alexanderduyck@fb.com>,
        "Jesper Dangaard Brouer" <hawk@kernel.org>,
        Ilias Apalodimas <ilias.apalodimas@linaro.org>,
        <netdev@vger.kernel.org>, <linux-kernel@vger.kernel.org>
References: <20230629152305.905962-1-aleksander.lobakin@intel.com>
 <20230629152305.905962-3-aleksander.lobakin@intel.com>
 <69e827e239dab9fd7986ee43cef599d024c8535f.camel@gmail.com>
 <ac4a8761-410e-e8cc-d6b2-d56b820a7888@intel.com>
 <CAKgT0UfZCGnWgOH96E4GV3ZP6LLbROHM7SHE8NKwq+exX+Gk_Q@mail.gmail.com>
From:   Alexander Lobakin <aleksander.lobakin@intel.com>
In-Reply-To: <CAKgT0UfZCGnWgOH96E4GV3ZP6LLbROHM7SHE8NKwq+exX+Gk_Q@mail.gmail.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 8bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?emZ3ZWFvV2V3ZnlIdWlYQWJSdzVpRHQ0TGdtOXpKK1lwRDRYSTVOSXFzRFMz?=
 =?utf-8?B?UmlDL3JvbmRnVHpaVE1hVENVdzJNY2JoYVdYZW5oc0ZOa25KNVBHQWtzT2dG?=
 =?utf-8?B?djM4Q3dBYUpvT2htcGRrbzYyZkVTUWpYQW9sMjFHMkZwVzlEL3QwN3B3U2Ix?=
 =?utf-8?B?b2l6bFRvWVR3cjc1eUg0c2p2OUw0eFNYVkdEV1YyS1hoNWxjOGY1TTIwRWhQ?=
 =?utf-8?B?cGJQdHJSM3FTMzZ4MTErSXhqY092WHoycDBsRXN2ZFJoeUJwbm8vUnNXaURk?=
 =?utf-8?B?RHpEd01uV1o0b2M2SXJ3TG83Vkl3MTdZWDJvc0ZzZzhUOVhsdkVVMml2MlZX?=
 =?utf-8?B?SXo1R21YbFpTSTloV0FXcGZiUC8vRXZMQk5NSEdFNjB2OVFicFNjV1dxM1lF?=
 =?utf-8?B?MEMxdjJEMlFRWWt4RGE0b0tEQi81bVBmK0ZHekZsYnFhR1JJck10MkNHZzVB?=
 =?utf-8?B?VHZWSzJjMVFqSU1ncFB6d2VZdHk3WFdNelJTb0lua0R3LzVZcE14R0ZvNU01?=
 =?utf-8?B?REQwMVJTdFhUWXdNeEhwVm53WWVXa3I1SFVMR2FkNE43ZEpna1ZQRUhXdko2?=
 =?utf-8?B?ZXVESjBtOUFXMmlNbVBXOWd4Vlg2ZFQ4aW43RUVpSmRoSUZvYmV2MmxsWDVz?=
 =?utf-8?B?MUJySmhRZGlVK0pmOTZHMytPNFVoQStOVlJja1RiU21sWUxTTTY0M0JObXF1?=
 =?utf-8?B?TGdXWTRqZk9FenpyemVhTnZXaXpCclBjaVZYRjRPYVc1ZXppaU5LbWpwTjRM?=
 =?utf-8?B?cC9aY2ZKc2h2R2ZsQUVuSVhmeHZxU05wUm9JWktRQnBjcFJNZ1RaaG9xTVV3?=
 =?utf-8?B?K3FTWC9lRHVLM09ienNLL1A5aDh3dFIwL0J5Z3ZrTlZRRVM2WUtNajB5Q29H?=
 =?utf-8?B?ZHQvckZHdnhJRnhLUitna0x4NVd4TUFHYXM3TFNNenBYTFBNYUFxWWFJRE0v?=
 =?utf-8?B?aFVLeUVqeXl1OEJXZUlweE5lUWdhNnZBeHk2em5vdHpLaHpJa0FONVY4NEZF?=
 =?utf-8?B?Skg2ZENMRlpkQ2lYQTNnczljKzlta3E2eWdGNUgyYzJSdkdWdFFhamVrQWJX?=
 =?utf-8?B?OHQ4YzhmRDVlVUN3VVR6UGt4TURHZzZlN0F4QlNIemVYMWZxbFNuTFF3cjZ5?=
 =?utf-8?B?Y080YngwanBJRGNSUnoyMjZmMlhERHh4ZmlkV0tZR0hTNGRjVjZrSnJNcTZK?=
 =?utf-8?B?UlNpUUFmeFBpWVNobTNob2x0SmhubFVGTkpEaGVFYlRDZnRpMVNkZmtSbU1y?=
 =?utf-8?B?YVc2SytZQ0RXMjVaR2xEWTJEWnVmM0E1WTdWTFhCZEhGMGc2ZDhvcFozN1hE?=
 =?utf-8?B?NzlWY2FYNDAvMmtHVk5KY0ZUQy9kNWpKbWkrMTFUTWFRdkwvOHg5dG1QL25J?=
 =?utf-8?B?bm44dXJwSE9hb1E1byt1SWhjb1AvL0ErNGpEd2NUREMvNzhXNUU5ZWpqZm5y?=
 =?utf-8?B?eTRRZVZVRDF1UFRHZnowcDJUSnBCaC92RkdBbkwrVVc2QlBzVnhzUnp4aDZE?=
 =?utf-8?B?YjIxN3Nod3pLSVB6aWwzVWswcEVDd05QdG1ReU5WcVR4dTBmSlBFVG9EZnAw?=
 =?utf-8?B?MU5mRG8xODZrbHJoTFBtTlFkNDBsN0I0SlFvMTFLam0xQm1idFo2R3piSlIy?=
 =?utf-8?B?VENqU1NIRnNmYWlSelFtSnExM1RjdmFia1FrZEhlQ294dGRzUEVQa25iMFNI?=
 =?utf-8?B?KzNDMHBPR1hpQUpYYnBnWmtqcXhKclJsRjRnSXBwamJPN200dk5neExBMzRp?=
 =?utf-8?B?ZXVFcXdpQll4Tk9yWERuYjV1N2JwS2FPRjlKVDB0cnJSOGJsdUVWMm5Kci9U?=
 =?utf-8?B?OHdpN1QwVlJoc0pDM3JpakZUZjNDT2lhUkZyeWh2QllaTFp4TWNDc2xBSzNu?=
 =?utf-8?B?b2lkNXdYRUd5dmpXWFRMaVZaQ1p5QjJtQjlieGdEQVJKb0o4Umh3bXZVK0xi?=
 =?utf-8?B?dFVoVTh0M3dOS2M4YUlVbnZtNFNrOURzdkVFdjVjQTJTODZTQUZFUmFhNy9k?=
 =?utf-8?B?ZGd4TkQ3cFNZVXFERDl4Z3QwNU56ZTZwWWRaYWxWZkprbGppK2VBSXV2M3VQ?=
 =?utf-8?B?aFZUaFBCUGtDdWx5dkNRSTVSNVNScjVMY0NRZ1FPdHQrUE1VR1VxdWgrU0JW?=
 =?utf-8?B?YlNHN1FtWHdkalVhS3FPN3R0WGVOcVJGZWR3aTJ3UEo1Z3IzU1hxWUZCRGcv?=
 =?utf-8?B?TFE9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 973889a0-833c-4c43-8ee5-08db797f8607
X-MS-Exchange-CrossTenant-AuthSource: DM6PR11MB3625.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 30 Jun 2023 15:34:42.1825
 (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: zxaOlqgP/k1WeBIwae5Pid38cCNVU+CT1jancyeM2cGKXG+rPiRUAUUEV8Bb7QvwCyRpmeJ8OqEP/FuW3hZt2s4cqr5EIK1orB162wqpUX4=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CY8PR11MB7873
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-4.5 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,
        RCVD_IN_DNSWL_MED,SPF_HELO_NONE,SPF_NONE,T_SCC_BODY_TEXT_LINE,
        URIBL_BLOCKED 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

From: Alexander Duyck <alexander.duyck@gmail.com>
Date: Fri, 30 Jun 2023 07:44:45 -0700

> On Fri, Jun 30, 2023 at 5:30 AM Alexander Lobakin
> <aleksander.lobakin@intel.com> wrote:
>>
>> From: Alexander H Duyck <alexander.duyck@gmail.com>
>> Date: Thu, 29 Jun 2023 09:45:26 -0700
>>
>>> On Thu, 2023-06-29 at 17:23 +0200, Alexander Lobakin wrote:
>>>> Turned out page_pool_put{,_full}_page() can burn quite a bunch of cycles
>>>> even when on DMA-coherent platforms (like x86) with no active IOMMU or
>>>> swiotlb, just for the call ladder.
>>>> Indeed, it's
>>
>> [...]
>>
>>>> @@ -341,6 +345,12 @@ static bool page_pool_dma_map(struct page_pool *pool, struct page *page)
>>>>
>>>>      page_pool_set_dma_addr(page, dma);
>>>>
>>>> +    if ((pool->p.flags & PP_FLAG_DMA_MAYBE_SYNC) &&
>>>> +        dma_need_sync(pool->p.dev, dma)) {
>>>> +            pool->p.flags |= PP_FLAG_DMA_SYNC_DEV;
>>>> +            pool->p.flags &= ~PP_FLAG_DMA_MAYBE_SYNC;
>>>> +    }
>>>> +
>>>>      if (pool->p.flags & PP_FLAG_DMA_SYNC_DEV)
>>>>              page_pool_dma_sync_for_device(pool, page, pool->p.max_len);
>>>>
>>>
>>> I am pretty sure the logic is flawed here. The problem is
>>> dma_needs_sync depends on the DMA address being used. In the worst case
>>> scenario we could have a device that has something like a 32b DMA
>>> address space on a system with over 4GB of memory. In such a case the
>>> higher addresses would need to be synced because they will go off to a
>>> swiotlb bounce buffer while the lower addresses wouldn't.
>>>
>>> If you were to store a flag like this it would have to be generated per
>>> page.
>>
>> I know when DMA might need syncing :D That's the point of this shortcut:
>> if at least one page needs syncing, I disable it for the whole pool.
>> It's a "better safe than sorry".
>> Using a per-page flag involves more changes and might hurt some
>> scenarios/setups. For example, non-coherent systems, where you always
>> need to do syncs. The idea was to give some improvement when possible,
>> otherwise just fallback to what we have today.
> 
> I am not a fan of having the page pool force the syncing either. Last
> I knew I thought the PP_FLAG_DMA_SYNC_DEV was meant to be set by the

Please follow the logics of the patch.

1. The driver sets DMA_SYNC_DEV.
2. PP tries to shortcut and replaces it with MAYBE_SYNC.
3. If dma_need_sync() returns true for some page, it gets replaced back
   to DMA_SYNC_DEV, no further dma_need_sync() calls for that pool.

OR

1. The driver doesn't set DMA_SYNC_DEV.
2. PP doesn't turn on MAYBE_SYNC.
3. No dma_need_sync() tests.

Where does PP force syncs for drivers which don't need them?

> driver, not by the page pool API itself. The big reason for that being
> that the driver in many cases will have to take care of the DMA sync
> itself instead of letting the allocator take care of it.
> 
> Basically we are just trading off the dma_need_sync cost versus the
> page_pool_dma_sync_for_device cost. If we think it is a win to call

dma_need_sync() is called once per newly allocated and mapped page.
page_pool_dma_sync_for_device() is called each time you ask for a page.

On my setup and with patch #4, I have literally 0 allocations once a
ring is filled. This means dma_need_sync() is not called at all during
Rx, while sync_for_device() would be called all the time.
When pages go through ptr_ring, sometimes new allocations happen, but
still the number of times dma_need_sync() is called is thousands times
lower.

> dma_need_sync for every frame then maybe we should look at folding it
> into page_pool_dma_sync_for_device itself since that is the only
> consumer of it it or just fold it into the PP_FLAG_DMA_SYNC_DEV if
> statement after the flag check rather than adding yet another flag
> that will likely always be true for most devices. Otherwise you are

What you suggest is either calling dma_need_sync() each time a page is
requested or introducing a flag to store it somewhere in struct page to
allow some optimization for really-not-common-cases when dma_need_sync()
might return different values due to swiotlb etc. Did I get it right?

> just adding overhead for the non-exception case and devices that don't
> bother setting PP_FLAG_DMA_SYNC_DEV.

Thanks,
Olek