Page Pool for IAVF: libie edition by alobakin · Pull Request #18 · alobakin/linux

alobakin · 2023-03-09T14:43:22Z

net: intel: start The Great Code Dedup + Page Pool for iavf

Here's a two-shot: introduce Intel Ethernet common library (libie) and
switch iavf to Page Pool. Details in the commit messages; here's the
summary:

Not a secret there's a ton of code duplication betwee two and more Intel
ethernet modules. Before introducing new changes, which would need to be
copied over again, start decoupling the already existing duplicate
functionality into a new module, which will be shared between several
Intel Ethernet drivers.
The first thing that came to my mind was "libie" -- "Intel Ethernet
common library". Also this sounds like "lovelie" and can be expanded as
"lib Internet Explorer" :P I'm open for anything else (but justified).
The series is only the beginning. From now on, adding every new feature
or doing any good driver refactoring will remove much more lines than add
for quite some time. There's a basic roadmap with some deduplications
planned already, not speaking of that touching every line now asks: "can
I share this?".
PP conversion for iavf lands within the same series as these two are tied
closely. libie will support Page Pool model only, so a driver can't use
much of the lib until it's converted. iavf is only the example, the rest
will eventually be converted soon on a per-driver basis. That is when it
will get really interesting. Stay tech.

jbrandeb · 2023-03-09T17:42:59Z

-		page_pool_release_page(xa->page_pool, page);
-	rcu_read_unlock();
-}
-EXPORT_SYMBOL_GPL(__xdp_release_frame);


since this was an export symbol should you declare it deprecated first and then remove the code in a future kernel?

This commit is unrelated to the series, it shows up here automatically since GH compares it to net-next. But I doubt is was ever used anywhere.

jbrandeb · 2023-03-09T17:43:41Z

 			    IAVF_RX_DESC_STATUS_FLTSTAT_SHIFT);

-	if (ring->netdev->features & NETIF_F_RXHASH)
+	if (!(ring->netdev->features & NETIF_F_RXHASH))


This is a fix? why include in this series?

pending fixes and improvements from LKML

It's part of that

Ah, I see now there was a note in the cover letter (5 fixes) that I had missed. Thanks!

Ok, I see I missed that there are 5 fixes commits mentioned in the cover letter, thanks!

jbrandeb · 2023-03-09T17:49:12Z

-
-	skb_checksum_none_assert(skb);
-
-	/* Rx csum enabled and ip headers found? */


Why remove this check? The idea here was to no longer report checksum good from the driver if the stack feature is not enabled. We can't stop the hardware from checking (or dont' want to reconfigure) but it's ok not to report.

Cool, i see you moved the check into libie return from libie_has_rx_csum

Yes, it's generic now.
And if anybody is curious about skb_checksum_none_assert(): it's completely useless here, since we were explicitly setting the checksum to NONE a line before... ._.

jbrandeb · 2023-03-09T17:51:31Z

+/* SPDX-License-Identifier: GPL-2.0-only */
+
+#ifndef __LINUX_INTEL_LIBIE_H
+#define __LINUX_INTEL_LIBIE_H


don't these guards usually use __FOO__ ?

It depends. I don't remember any strong rule. But if we have ones at Intel, let me know and I'll fix.
Most people nowadays prefer __FOO__ though I think.

Yeah, I'd prefer you fixed this.

jbrandeb · 2023-03-09T18:13:54Z

-	}
-
-	for (i = 0; i < adapter->num_active_queues; i++) {
+	for (u32 i = 0; i < adapter->num_active_queues; i++)


hahaha, c99 abuser :-P (I prefer this anyway.)

Yeah, I find this Linus-propored-c11-switch very useful. It limits the scope of the iterators, so you can't abuse it outside the loop, like a ton of devs was doing.

jbrandeb · 2023-03-09T18:14:22Z


 	netdev->netdev_ops = &iavf_netdev_ops;
 	iavf_set_ethtool_ops(netdev);
-	netdev->watchdog_timeo = 5 * HZ;


why remove watchdog_timeo?

5 HZ (5 seconds) is default, you can grep by HZ in net/sched. So manual setting to 5 sec makes no sense. The other thing is that it's not related to the actual commit, but will see if anyone objects :D

jbrandeb · 2023-03-09T18:15:53Z

 		    !test_bit(__IAVF_VSI_DOWN, vsi->state) &&
 		    (IAVF_DESC_UNUSED(tx_ring) != tx_ring->count))
-			tx_ring->arm_wb = true;
+			tx_ring->flags |= IAVF_TXRX_FLAGS_ARM_WB;


The arm_wb stuff should have been part of virtchnl.h as it's not required on ice. :-(

So it can even be removed at all? That would be even better :D

Arm WB has to be there for iavf to work correctly for fortville/i40e based VFs. So you can't remove it. :-(

jbrandeb · 2023-03-09T18:27:28Z

 	rx_ring->rx_pages = NULL;

+	page_pool_destroy(rx_ring->pool);
+	rx_ring->dev = dev;


I don't get it, why do this in clean_rx_ring?

I keep the lifetime of the pool the same as the lifetime of the Rx page array. This way I'll never miss to destroy/create/allocate something. Page Pool entity itself is intended to have this lifetime.

(the other thing is that we need to store Page Pool stats manually between ifdowns/ups, but I don't manage stats in this series yet)

jbrandeb · 2023-03-09T18:29:12Z

I like where this is going! Looks really good.

jbrandeb · 2023-03-15T00:56:14Z

 				(0x1ULL << ICE_FXD_FLTR_WB_QW1_FAIL_PROF_S)
 #define ICE_FXD_FLTR_WB_QW1_FAIL_PROF_YES	0x1ULL

-struct ice_rx_ptype_decoded {


This ptype decoding stuff into a bitfield is what you have to make sure we don't use any more in libie.

jbrandeb · 2023-03-15T00:58:03Z

+ * bitfield struct.
+ */
+
+struct libie_rx_ptype_parsed {


I don't think you can count on this struct with the ptype using bitfields to be true on ice. We can look over the changes in the manual to see if they appear to be.

I had a look at the manual, it looks like the first 256 ptypes are kept compatible. For more information (and it will help with this ptype code if you haven't seen it) in the public manual here: https://www.intel.com/content/www/us/en/content-details/613875/ And see Appendix A. It's totally apropos to making sure we have the right info that's backwards compatible in libie.

irq_cpu_rmap_release() calls cpu_rmap_put(), which may free the rmap. So we need to clear the pointer to our glue structure in rmap before doing that, not after. Fixes: 4e0473f ("lib: cpu_rmap: Avoid use after free on rmap->obj array entries") Signed-off-by: Ben Hutchings <ben@decadent.org.uk> Reviewed-by: Simon Horman <simon.horman@corigine.com> Link: https://lore.kernel.org/r/ZHo0vwquhOy3FaXc@decadent.org.uk Signed-off-by: Jakub Kicinski <kuba@kernel.org>

Expand the libie generic per-queue stats with the generic Page Pool stats provided by the API itself, when CONFIG_PAGE_POOL is enabled. When it's not, there'll be no such fields in the stats structure, so no space wasted. They are also a bit special in terms of how they are obtained. One &page_pool accumulates statistics until it's destroyed obviously, which happens on ifdown. So, in order to not lose any statistics, get the stats and store in the queue container before destroying a pool. This container survives ifups/downs, so it basically stores the statistics accumulated since the very first pool was allocated on this queue. When it's needed to export the stats, first get the numbers from this container and then add the "live" numbers -- the ones that the current active pool returns. The result values will always represent the actual device-lifetime* stats. There's a cast from &page_pool_stats to `u64 *` in a couple functions, but they are guarded with stats asserts to make sure it's safe to do. FWIW it saves a lot of object code. Reviewed-by: Paul Menzel <pmenzel@molgen.mpg.de> Signed-off-by: Alexander Lobakin <aleksander.lobakin@intel.com>

iavf is pretty much ready for using the generic libie stats, so drop all the custom code and just use generic definitions. The only thing is that it previously lacked the counter of Tx queue stops. It's present in the other drivers, so add it here as well. The rest is straightforward. There were two fields in the Tx stats struct, which didn't belong there. The first one has never been used, wipe it; and move the other to the queue structure. Plus move around a couple fields in &iavf_ring to account stats structs' alignment. Signed-off-by: Alexander Lobakin <aleksander.lobakin@intel.com>

jbrandeb reviewed Mar 9, 2023

View reviewed changes

alobakin force-pushed the iavf-pp branch 3 times, most recently from 1afc0af to c8b0fb0 Compare March 10, 2023 09:40

jbrandeb reviewed Mar 15, 2023

View reviewed changes

alobakin force-pushed the iavf-pp branch from c8b0fb0 to b5f3a1a Compare March 24, 2023 18:44

alobakin force-pushed the iavf-pp branch 3 times, most recently from 99e1f72 to c5c2199 Compare May 15, 2023 15:39

alobakin force-pushed the iavf-pp branch 10 times, most recently from 88f0285 to b1f2c3b Compare May 22, 2023 14:03

alobakin force-pushed the iavf-pp branch from b1f2c3b to 1ab451a Compare May 30, 2023 13:54

alobakin added 2 commits June 13, 2023 16:46

alobakin force-pushed the iavf-pp branch from 1ab451a to 0109d80 Compare June 13, 2023 14:47

alobakin closed this Apr 4, 2024

alobakin deleted the iavf-pp branch April 4, 2024 09:56


		skb_checksum_none_assert(skb);

		/* Rx csum enabled and ip headers found? */

Conversation

alobakin commented Mar 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alobakin Mar 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jbrandeb Mar 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jbrandeb commented Mar 9, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

20 participants

alobakin commented Mar 9, 2023 •

edited

Loading

alobakin Mar 14, 2023 •

edited

Loading

jbrandeb Mar 9, 2023 •

edited

Loading