| Message ID | 20240403002053.2376017-8-almasrymina@google.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [RFC,net-next,v8,01/14] queue_api: define queue api | expand |
On 2024-04-02 5:20 pm, Mina Almasry wrote: > @@ -69,20 +106,26 @@ net_iov_binding(const struct net_iov *niov) > */ > typedef unsigned long __bitwise netmem_ref; > > +static inline bool netmem_is_net_iov(const netmem_ref netmem) > +{ > +#if defined(CONFIG_PAGE_POOL) && defined(CONFIG_DMA_SHARED_BUFFER) I am guessing you added this to try and speed up the fast path? It's overly restrictive for us since we do not need dmabuf necessarily. I spent a bit too much time wondering why things aren't working only to find this :(
On Fri, Apr 26, 2024 at 5:18 PM David Wei <dw@davidwei.uk> wrote: > > On 2024-04-02 5:20 pm, Mina Almasry wrote: > > @@ -69,20 +106,26 @@ net_iov_binding(const struct net_iov *niov) > > */ > > typedef unsigned long __bitwise netmem_ref; > > > > +static inline bool netmem_is_net_iov(const netmem_ref netmem) > > +{ > > +#if defined(CONFIG_PAGE_POOL) && defined(CONFIG_DMA_SHARED_BUFFER) > > I am guessing you added this to try and speed up the fast path? It's > overly restrictive for us since we do not need dmabuf necessarily. I > spent a bit too much time wondering why things aren't working only to > find this :( My apologies, I'll try to put the changelog somewhere prominent, or notify you when I do something that I think breaks you. Yes, this is a by-product of a discussion with regards to the page_pool benchmark regressions due to adding devmem. There is some background on why this was added and the impact on the bench_page_pool_simple tests in the cover letter. For you, I imagine you want to change this to something like: #if defined(CONFIG_PAGE_POOL) #if defined(CONFIG_DMA_SHARED_BUFFER) || defined(CONFIG_IOURING) or something like that, right? Not sure if this is something I should do here or if something more appropriate to be in the patches you apply on top. I additionally think you may also need to run the page_pool_benchmark_simple tests like I do in the cover letter to see if you're affecting those. -- Thanks, Mina
On 4/27/24 03:11, Mina Almasry wrote: > On Fri, Apr 26, 2024 at 5:18 PM David Wei <dw@davidwei.uk> wrote: >> >> On 2024-04-02 5:20 pm, Mina Almasry wrote: >>> @@ -69,20 +106,26 @@ net_iov_binding(const struct net_iov *niov) >>> */ >>> typedef unsigned long __bitwise netmem_ref; >>> >>> +static inline bool netmem_is_net_iov(const netmem_ref netmem) >>> +{ >>> +#if defined(CONFIG_PAGE_POOL) && defined(CONFIG_DMA_SHARED_BUFFER) >> >> I am guessing you added this to try and speed up the fast path? It's >> overly restrictive for us since we do not need dmabuf necessarily. I >> spent a bit too much time wondering why things aren't working only to >> find this :( > > My apologies, I'll try to put the changelog somewhere prominent, or > notify you when I do something that I think breaks you. > > Yes, this is a by-product of a discussion with regards to the > page_pool benchmark regressions due to adding devmem. There is some > background on why this was added and the impact on the > bench_page_pool_simple tests in the cover letter. > > For you, I imagine you want to change this to something like: > > #if defined(CONFIG_PAGE_POOL) > #if defined(CONFIG_DMA_SHARED_BUFFER) || defined(CONFIG_IOURING) > > or something like that, right? Not sure if this is something I should Feels a bit flimsy, if the argument is that you want to be able to disable netmem overhead, then adding a netmem config option sounds like a better way forward. I have doubts this conditional handling is desirable in the first place, but perhaps I missed the discussion. > do here or if something more appropriate to be in the patches you > apply on top. > > I additionally think you may also need to run the > page_pool_benchmark_simple tests like I do in the cover letter to see > if you're affecting those.
On 4/26/24 8:11 PM, Mina Almasry wrote: > On Fri, Apr 26, 2024 at 5:18?PM David Wei <dw@davidwei.uk> wrote: >> >> On 2024-04-02 5:20 pm, Mina Almasry wrote: >>> @@ -69,20 +106,26 @@ net_iov_binding(const struct net_iov *niov) >>> */ >>> typedef unsigned long __bitwise netmem_ref; >>> >>> +static inline bool netmem_is_net_iov(const netmem_ref netmem) >>> +{ >>> +#if defined(CONFIG_PAGE_POOL) && defined(CONFIG_DMA_SHARED_BUFFER) >> >> I am guessing you added this to try and speed up the fast path? It's >> overly restrictive for us since we do not need dmabuf necessarily. I >> spent a bit too much time wondering why things aren't working only to >> find this :( > > My apologies, I'll try to put the changelog somewhere prominent, or > notify you when I do something that I think breaks you. > > Yes, this is a by-product of a discussion with regards to the > page_pool benchmark regressions due to adding devmem. There is some > background on why this was added and the impact on the > bench_page_pool_simple tests in the cover letter. > > For you, I imagine you want to change this to something like: > > #if defined(CONFIG_PAGE_POOL) > #if defined(CONFIG_DMA_SHARED_BUFFER) || defined(CONFIG_IOURING) > > or something like that, right? Not sure if this is something I should > do here or if something more appropriate to be in the patches you > apply on top. In general, attempting to hide overhead behind config options is always a losing proposition. It merely serves to say "look, if these things aren't enabled, the overhead isn't there", while distros blindly enable pretty much everything and then you're back where you started.
On Tue, Apr 30, 2024 at 6:46 AM Jens Axboe <axboe@kernel.dk> wrote: > > On 4/26/24 8:11 PM, Mina Almasry wrote: > > On Fri, Apr 26, 2024 at 5:18?PM David Wei <dw@davidwei.uk> wrote: > >> > >> On 2024-04-02 5:20 pm, Mina Almasry wrote: > >>> @@ -69,20 +106,26 @@ net_iov_binding(const struct net_iov *niov) > >>> */ > >>> typedef unsigned long __bitwise netmem_ref; > >>> > >>> +static inline bool netmem_is_net_iov(const netmem_ref netmem) > >>> +{ > >>> +#if defined(CONFIG_PAGE_POOL) && defined(CONFIG_DMA_SHARED_BUFFER) > >> > >> I am guessing you added this to try and speed up the fast path? It's > >> overly restrictive for us since we do not need dmabuf necessarily. I > >> spent a bit too much time wondering why things aren't working only to > >> find this :( > > > > My apologies, I'll try to put the changelog somewhere prominent, or > > notify you when I do something that I think breaks you. > > > > Yes, this is a by-product of a discussion with regards to the > > page_pool benchmark regressions due to adding devmem. There is some > > background on why this was added and the impact on the > > bench_page_pool_simple tests in the cover letter. > > > > For you, I imagine you want to change this to something like: > > > > #if defined(CONFIG_PAGE_POOL) > > #if defined(CONFIG_DMA_SHARED_BUFFER) || defined(CONFIG_IOURING) > > > > or something like that, right? Not sure if this is something I should > > do here or if something more appropriate to be in the patches you > > apply on top. > > In general, attempting to hide overhead behind config options is always > a losing proposition. It merely serves to say "look, if these things > aren't enabled, the overhead isn't there", while distros blindly enable > pretty much everything and then you're back where you started. > The history there is that this check adds 1 cycle regression to the page_pool fast path benchmark. The regression last I measured is 8->9 cycles, so in % wise it's a quite significant 12.5% (more details in the cover letter[1]). I doubt I can do much better than that to be honest. There was a desire not to pay this overhead in setups that will likely not care about devmem, like embedded devices maybe, or setups without GPUs. Adding a CONFIG check here seemed like very low hanging fruit, but yes it just hides the overhead in some configs, not really removes it. There was a discussion about adding this entire netmem/devmem work under a new CONFIG. There was pushback particularly from Willem that at the end of the day what is enabled on most distros is what matters and we added code churn and CONFIG churn for little value. If there is significant pushback to the CONFIG check I can remove it. I don't feel like it's critical, it just mirco-optimizes some setups that doesn't really care about this work area. [1] https://lore.kernel.org/netdev/20240403002053.2376017-1-almasrymina@google.com/
On 4/30/24 12:29 PM, Mina Almasry wrote: > On Tue, Apr 30, 2024 at 6:46?AM Jens Axboe <axboe@kernel.dk> wrote: >> >> On 4/26/24 8:11 PM, Mina Almasry wrote: >>> On Fri, Apr 26, 2024 at 5:18?PM David Wei <dw@davidwei.uk> wrote: >>>> >>>> On 2024-04-02 5:20 pm, Mina Almasry wrote: >>>>> @@ -69,20 +106,26 @@ net_iov_binding(const struct net_iov *niov) >>>>> */ >>>>> typedef unsigned long __bitwise netmem_ref; >>>>> >>>>> +static inline bool netmem_is_net_iov(const netmem_ref netmem) >>>>> +{ >>>>> +#if defined(CONFIG_PAGE_POOL) && defined(CONFIG_DMA_SHARED_BUFFER) >>>> >>>> I am guessing you added this to try and speed up the fast path? It's >>>> overly restrictive for us since we do not need dmabuf necessarily. I >>>> spent a bit too much time wondering why things aren't working only to >>>> find this :( >>> >>> My apologies, I'll try to put the changelog somewhere prominent, or >>> notify you when I do something that I think breaks you. >>> >>> Yes, this is a by-product of a discussion with regards to the >>> page_pool benchmark regressions due to adding devmem. There is some >>> background on why this was added and the impact on the >>> bench_page_pool_simple tests in the cover letter. >>> >>> For you, I imagine you want to change this to something like: >>> >>> #if defined(CONFIG_PAGE_POOL) >>> #if defined(CONFIG_DMA_SHARED_BUFFER) || defined(CONFIG_IOURING) >>> >>> or something like that, right? Not sure if this is something I should >>> do here or if something more appropriate to be in the patches you >>> apply on top. >> >> In general, attempting to hide overhead behind config options is always >> a losing proposition. It merely serves to say "look, if these things >> aren't enabled, the overhead isn't there", while distros blindly enable >> pretty much everything and then you're back where you started. >> > > The history there is that this check adds 1 cycle regression to the > page_pool fast path benchmark. The regression last I measured is 8->9 > cycles, so in % wise it's a quite significant 12.5% (more details in > the cover letter[1]). I doubt I can do much better than that to be > honest. I'm all for cycle counting, and do it myself too, but is that even measurable in anything that isn't a super targeted microbenchmark? Or even in that? > There was a desire not to pay this overhead in setups that will likely > not care about devmem, like embedded devices maybe, or setups without > GPUs. Adding a CONFIG check here seemed like very low hanging fruit, > but yes it just hides the overhead in some configs, not really removes > it. > > There was a discussion about adding this entire netmem/devmem work > under a new CONFIG. There was pushback particularly from Willem that > at the end of the day what is enabled on most distros is what matters > and we added code churn and CONFIG churn for little value. > > If there is significant pushback to the CONFIG check I can remove it. > I don't feel like it's critical, it just mirco-optimizes some setups > that doesn't really care about this work area. That is true, but in practice it'll be enabled anyway. Seems like it's not really worth it in this scenario.
On Tue, Apr 30, 2024 at 11:55 AM Jens Axboe <axboe@kernel.dk> wrote: > > On 4/30/24 12:29 PM, Mina Almasry wrote: > > On Tue, Apr 30, 2024 at 6:46?AM Jens Axboe <axboe@kernel.dk> wrote: > >> > >> On 4/26/24 8:11 PM, Mina Almasry wrote: > >>> On Fri, Apr 26, 2024 at 5:18?PM David Wei <dw@davidwei.uk> wrote: > >>>> > >>>> On 2024-04-02 5:20 pm, Mina Almasry wrote: > >>>>> @@ -69,20 +106,26 @@ net_iov_binding(const struct net_iov *niov) > >>>>> */ > >>>>> typedef unsigned long __bitwise netmem_ref; > >>>>> > >>>>> +static inline bool netmem_is_net_iov(const netmem_ref netmem) > >>>>> +{ > >>>>> +#if defined(CONFIG_PAGE_POOL) && defined(CONFIG_DMA_SHARED_BUFFER) > >>>> > >>>> I am guessing you added this to try and speed up the fast path? It's > >>>> overly restrictive for us since we do not need dmabuf necessarily. I > >>>> spent a bit too much time wondering why things aren't working only to > >>>> find this :( > >>> > >>> My apologies, I'll try to put the changelog somewhere prominent, or > >>> notify you when I do something that I think breaks you. > >>> > >>> Yes, this is a by-product of a discussion with regards to the > >>> page_pool benchmark regressions due to adding devmem. There is some > >>> background on why this was added and the impact on the > >>> bench_page_pool_simple tests in the cover letter. > >>> > >>> For you, I imagine you want to change this to something like: > >>> > >>> #if defined(CONFIG_PAGE_POOL) > >>> #if defined(CONFIG_DMA_SHARED_BUFFER) || defined(CONFIG_IOURING) > >>> > >>> or something like that, right? Not sure if this is something I should > >>> do here or if something more appropriate to be in the patches you > >>> apply on top. > >> > >> In general, attempting to hide overhead behind config options is always > >> a losing proposition. It merely serves to say "look, if these things > >> aren't enabled, the overhead isn't there", while distros blindly enable > >> pretty much everything and then you're back where you started. > >> > > > > The history there is that this check adds 1 cycle regression to the > > page_pool fast path benchmark. The regression last I measured is 8->9 > > cycles, so in % wise it's a quite significant 12.5% (more details in > > the cover letter[1]). I doubt I can do much better than that to be > > honest. > > I'm all for cycle counting, and do it myself too, but is that even > measurable in anything that isn't a super targeted microbenchmark? Or > even in that? > Not as far as I can tell, no. This was purely to improve the page_pool benchmark. > > There was a desire not to pay this overhead in setups that will likely > > not care about devmem, like embedded devices maybe, or setups without > > GPUs. Adding a CONFIG check here seemed like very low hanging fruit, > > but yes it just hides the overhead in some configs, not really removes > > it. > > > > There was a discussion about adding this entire netmem/devmem work > > under a new CONFIG. There was pushback particularly from Willem that > > at the end of the day what is enabled on most distros is what matters > > and we added code churn and CONFIG churn for little value. > > > > If there is significant pushback to the CONFIG check I can remove it. > > I don't feel like it's critical, it just mirco-optimizes some setups > > that doesn't really care about this work area. > > That is true, but in practice it'll be enabled anyway. Seems like it's > not really worth it in this scenario. > OK, no pushback from me. I'll remove the CONFIG check in the next iteration.
On Fri, Apr 26, 2024 at 05:17:52PM -0700, David Wei wrote: > On 2024-04-02 5:20 pm, Mina Almasry wrote: > > @@ -69,20 +106,26 @@ net_iov_binding(const struct net_iov *niov) > > */ > > typedef unsigned long __bitwise netmem_ref; > > > > +static inline bool netmem_is_net_iov(const netmem_ref netmem) > > +{ > > +#if defined(CONFIG_PAGE_POOL) && defined(CONFIG_DMA_SHARED_BUFFER) > > I am guessing you added this to try and speed up the fast path? It's > overly restrictive for us since we do not need dmabuf necessarily. I > spent a bit too much time wondering why things aren't working only to > find this :( So what else do you need? I was assured last round that nothing but dmabuf and potentially the huge page case (that really just is the page provider) would get added. > ---end quoted text---
On 30/04/2024 20.55, Jens Axboe wrote: > On 4/30/24 12:29 PM, Mina Almasry wrote: >> On Tue, Apr 30, 2024 at 6:46?AM Jens Axboe<axboe@kernel.dk> wrote: [...] >>> In general, attempting to hide overhead behind config options is always >>> a losing proposition. It merely serves to say "look, if these things >>> aren't enabled, the overhead isn't there", while distros blindly enable >>> pretty much everything and then you're back where you started. >>> >> The history there is that this check adds 1 cycle regression to the >> page_pool fast path benchmark. The regression last I measured is 8->9 >> cycles, so in % wise it's a quite significant 12.5% (more details in >> the cover letter[1]). I doubt I can do much better than that to be >> honest. > > I'm all for cycle counting, and do it myself too, but is that even > measurable in anything that isn't a super targeted microbenchmark? Or > even in that? The reason for page_pool fast path being critical is that it is used for the XDP_DROP use-case. E.g on Mellanox mlx5 driver we see 24 Mpps XDP_DROP, which is approx 42 nanosec per packet. Adding 9 nanosec will reduce this to 19.6 Mpps. 1/(42+9)*10^9 = 19607843 --Jesper p.s. Upstreaming my PP microbenchmark[1] is still at the bottom of my todo-list. [1] https://github.com/netoptimizer/prototype-kernel/blob/master/kernel/lib/bench_page_pool_simple.c
On 2024-05-01 00:55, Christoph Hellwig wrote: > On Fri, Apr 26, 2024 at 05:17:52PM -0700, David Wei wrote: >> On 2024-04-02 5:20 pm, Mina Almasry wrote: >>> @@ -69,20 +106,26 @@ net_iov_binding(const struct net_iov *niov) >>> */ >>> typedef unsigned long __bitwise netmem_ref; >>> >>> +static inline bool netmem_is_net_iov(const netmem_ref netmem) >>> +{ >>> +#if defined(CONFIG_PAGE_POOL) && defined(CONFIG_DMA_SHARED_BUFFER) >> >> I am guessing you added this to try and speed up the fast path? It's >> overly restrictive for us since we do not need dmabuf necessarily. I >> spent a bit too much time wondering why things aren't working only to >> find this :( > > So what else do you need? I was assured last round that nothing but > dmabuf and potentially the huge page case (that really just is the page > provider) would get added. I'm using userspace memory so having this gated behind CONFIG_DMA_SHARED_BUFFER doesn't make sense for us. > >> > ---end quoted text---
diff --git a/include/net/netmem.h b/include/net/netmem.h index 5f1c728618f2..74eeaa34883e 100644 --- a/include/net/netmem.h +++ b/include/net/netmem.h @@ -9,14 +9,51 @@ #define _NET_NETMEM_H #include <net/devmem.h> +#include <net/net_debug.h> /* net_iov */ +DECLARE_STATIC_KEY_FALSE(page_pool_mem_providers); + +/* We overload the LSB of the struct page pointer to indicate whether it's + * a page or net_iov. + */ +#define NET_IOV 0x01UL + struct net_iov { + unsigned long __unused_padding; + unsigned long pp_magic; + struct page_pool *pp; struct dmabuf_genpool_chunk_owner *owner; unsigned long dma_addr; + atomic_long_t pp_ref_count; }; +/* These fields in struct page are used by the page_pool and net stack: + * + * struct { + * unsigned long pp_magic; + * struct page_pool *pp; + * unsigned long _pp_mapping_pad; + * unsigned long dma_addr; + * atomic_long_t pp_ref_count; + * }; + * + * We mirror the page_pool fields here so the page_pool can access these fields + * without worrying whether the underlying fields belong to a page or net_iov. + * + * The non-net stack fields of struct page are private to the mm stack and must + * never be mirrored to net_iov. + */ +#define NET_IOV_ASSERT_OFFSET(pg, iov) \ + static_assert(offsetof(struct page, pg) == \ + offsetof(struct net_iov, iov)) +NET_IOV_ASSERT_OFFSET(pp_magic, pp_magic); +NET_IOV_ASSERT_OFFSET(pp, pp); +NET_IOV_ASSERT_OFFSET(dma_addr, dma_addr); +NET_IOV_ASSERT_OFFSET(pp_ref_count, pp_ref_count); +#undef NET_IOV_ASSERT_OFFSET + static inline struct dmabuf_genpool_chunk_owner * net_iov_owner(const struct net_iov *niov) { @@ -69,20 +106,26 @@ net_iov_binding(const struct net_iov *niov) */ typedef unsigned long __bitwise netmem_ref; +static inline bool netmem_is_net_iov(const netmem_ref netmem) +{ +#if defined(CONFIG_PAGE_POOL) && defined(CONFIG_DMA_SHARED_BUFFER) + return (__force unsigned long)netmem & NET_IOV; +#else + return false; +#endif +} + /* This conversion fails (returns NULL) if the netmem_ref is not struct page * backed. - * - * Currently struct page is the only possible netmem, and this helper never - * fails. */ static inline struct page *netmem_to_page(netmem_ref netmem) { + if (WARN_ON_ONCE(netmem_is_net_iov(netmem))) + return NULL; + return (__force struct page *)netmem; } -/* Converting from page to netmem is always safe, because a page can always be - * a netmem. - */ static inline netmem_ref page_to_netmem(struct page *page) { return (__force netmem_ref)page; @@ -90,17 +133,103 @@ static inline netmem_ref page_to_netmem(struct page *page) static inline int netmem_ref_count(netmem_ref netmem) { + /* The non-pp refcount of net_iov is always 1. On net_iov, we only + * support pp refcounting which uses the pp_ref_count field. + */ + if (netmem_is_net_iov(netmem)) + return 1; + return page_ref_count(netmem_to_page(netmem)); } static inline unsigned long netmem_to_pfn(netmem_ref netmem) { + if (netmem_is_net_iov(netmem)) + return 0; + return page_to_pfn(netmem_to_page(netmem)); } +static inline struct net_iov *__netmem_clear_lsb(netmem_ref netmem) +{ + return (struct net_iov *)((__force unsigned long)netmem & ~NET_IOV); +} + +static inline unsigned long netmem_get_pp_magic(netmem_ref netmem) +{ + return __netmem_clear_lsb(netmem)->pp_magic; +} + +static inline void netmem_or_pp_magic(netmem_ref netmem, unsigned long pp_magic) +{ + __netmem_clear_lsb(netmem)->pp_magic |= pp_magic; +} + +static inline void netmem_clear_pp_magic(netmem_ref netmem) +{ + __netmem_clear_lsb(netmem)->pp_magic = 0; +} + +static inline struct page_pool *netmem_get_pp(netmem_ref netmem) +{ + return __netmem_clear_lsb(netmem)->pp; +} + +static inline void netmem_set_pp(netmem_ref netmem, struct page_pool *pool) +{ + __netmem_clear_lsb(netmem)->pp = pool; +} + +static inline unsigned long netmem_get_dma_addr(netmem_ref netmem) +{ + return __netmem_clear_lsb(netmem)->dma_addr; +} + +static inline void netmem_set_dma_addr(netmem_ref netmem, + unsigned long dma_addr) +{ + __netmem_clear_lsb(netmem)->dma_addr = dma_addr; +} + +static inline atomic_long_t *netmem_get_pp_ref_count_ref(netmem_ref netmem) +{ + return &__netmem_clear_lsb(netmem)->pp_ref_count; +} + +static inline bool netmem_is_pref_nid(netmem_ref netmem, int pref_nid) +{ + /* Assume net_iov are on the preferred node without actually + * checking... + * + * This check is only used to check for recycling memory in the page + * pool's fast paths. Currently the only implementation of net_iov + * is dmabuf device memory. It's a deliberate decision by the user to + * bind a certain dmabuf to a certain netdev, and the netdev rx queue + * would not be able to reallocate memory from another dmabuf that + * exists on the preferred node, so, this check doesn't make much sense + * in this case. Assume all net_iovs can be recycled for now. + */ + if (netmem_is_net_iov(netmem)) + return true; + + return page_to_nid(netmem_to_page(netmem)) == pref_nid; +} + static inline netmem_ref netmem_compound_head(netmem_ref netmem) { + /* niov are never compounded */ + if (netmem_is_net_iov(netmem)) + return netmem; + return page_to_netmem(compound_head(netmem_to_page(netmem))); } +static inline void *netmem_address(netmem_ref netmem) +{ + if (netmem_is_net_iov(netmem)) + return NULL; + + return page_address(netmem_to_page(netmem)); +} + #endif /* _NET_NETMEM_H */ diff --git a/include/net/page_pool/helpers.h b/include/net/page_pool/helpers.h index 61814f91a458..c6a55eddefae 100644 --- a/include/net/page_pool/helpers.h +++ b/include/net/page_pool/helpers.h @@ -215,7 +215,7 @@ inline enum dma_data_direction page_pool_get_dma_dir(struct page_pool *pool) static inline void page_pool_fragment_netmem(netmem_ref netmem, long nr) { - atomic_long_set(&netmem_to_page(netmem)->pp_ref_count, nr); + atomic_long_set(netmem_get_pp_ref_count_ref(netmem), nr); } /** @@ -243,7 +243,7 @@ static inline void page_pool_fragment_page(struct page *page, long nr) static inline long page_pool_unref_netmem(netmem_ref netmem, long nr) { - struct page *page = netmem_to_page(netmem); + atomic_long_t *pp_ref_count = netmem_get_pp_ref_count_ref(netmem); long ret; /* If nr == pp_ref_count then we have cleared all remaining @@ -260,19 +260,19 @@ static inline long page_pool_unref_netmem(netmem_ref netmem, long nr) * initially, and only overwrite it when the page is partitioned into * more than one piece. */ - if (atomic_long_read(&page->pp_ref_count) == nr) { + if (atomic_long_read(pp_ref_count) == nr) { /* As we have ensured nr is always one for constant case using * the BUILD_BUG_ON(), only need to handle the non-constant case * here for pp_ref_count draining, which is a rare case. */ BUILD_BUG_ON(__builtin_constant_p(nr) && nr != 1); if (!__builtin_constant_p(nr)) - atomic_long_set(&page->pp_ref_count, 1); + atomic_long_set(pp_ref_count, 1); return 0; } - ret = atomic_long_sub_return(nr, &page->pp_ref_count); + ret = atomic_long_sub_return(nr, pp_ref_count); WARN_ON(ret < 0); /* We are the last user here too, reset pp_ref_count back to 1 to @@ -281,7 +281,7 @@ static inline long page_pool_unref_netmem(netmem_ref netmem, long nr) * page_pool_unref_page() currently. */ if (unlikely(!ret)) - atomic_long_set(&page->pp_ref_count, 1); + atomic_long_set(pp_ref_count, 1); return ret; } @@ -400,9 +400,7 @@ static inline void page_pool_free_va(struct page_pool *pool, void *va, static inline dma_addr_t page_pool_get_dma_addr_netmem(netmem_ref netmem) { - struct page *page = netmem_to_page(netmem); - - dma_addr_t ret = page->dma_addr; + dma_addr_t ret = netmem_get_dma_addr(netmem); if (PAGE_POOL_32BIT_ARCH_WITH_64BIT_DMA) ret <<= PAGE_SHIFT; @@ -425,18 +423,17 @@ static inline dma_addr_t page_pool_get_dma_addr(struct page *page) static inline bool page_pool_set_dma_addr_netmem(netmem_ref netmem, dma_addr_t addr) { - struct page *page = netmem_to_page(netmem); - if (PAGE_POOL_32BIT_ARCH_WITH_64BIT_DMA) { - page->dma_addr = addr >> PAGE_SHIFT; + netmem_set_dma_addr(netmem, addr >> PAGE_SHIFT); /* We assume page alignment to shave off bottom bits, * if this "compression" doesn't work we need to drop. */ - return addr != (dma_addr_t)page->dma_addr << PAGE_SHIFT; + return addr != (dma_addr_t)netmem_get_dma_addr(netmem) + << PAGE_SHIFT; } - page->dma_addr = addr; + netmem_set_dma_addr(netmem, addr); return false; } diff --git a/net/core/devmem.c b/net/core/devmem.c index 268fc8455a6d..c25ede5f6fb9 100644 --- a/net/core/devmem.c +++ b/net/core/devmem.c @@ -121,7 +121,10 @@ net_devmem_alloc_dmabuf(struct net_devmem_dmabuf_binding *binding) index = offset / PAGE_SIZE; niov = &owner->niovs[index]; + niov->pp_magic = 0; + niov->pp = NULL; niov->dma_addr = 0; + atomic_long_set(&niov->pp_ref_count, 0); net_devmem_dmabuf_binding_get(binding); diff --git a/net/core/page_pool.c b/net/core/page_pool.c index c8125be3a6e2..c7bffd08218b 100644 --- a/net/core/page_pool.c +++ b/net/core/page_pool.c @@ -25,7 +25,7 @@ #include "page_pool_priv.h" -static DEFINE_STATIC_KEY_FALSE(page_pool_mem_providers); +DEFINE_STATIC_KEY_FALSE(page_pool_mem_providers); #define DEFER_TIME (msecs_to_jiffies(1000)) #define DEFER_WARN_INTERVAL (60 * HZ) @@ -359,7 +359,7 @@ static noinline netmem_ref page_pool_refill_alloc_cache(struct page_pool *pool) if (unlikely(!netmem)) break; - if (likely(page_to_nid(netmem_to_page(netmem)) == pref_nid)) { + if (likely(netmem_is_pref_nid(netmem, pref_nid))) { pool->alloc.cache[pool->alloc.count++] = netmem; } else { /* NUMA mismatch; @@ -446,10 +446,8 @@ static bool page_pool_dma_map(struct page_pool *pool, netmem_ref netmem) static void page_pool_set_pp_info(struct page_pool *pool, netmem_ref netmem) { - struct page *page = netmem_to_page(netmem); - - page->pp = pool; - page->pp_magic |= PP_SIGNATURE; + netmem_set_pp(netmem, pool); + netmem_or_pp_magic(netmem, PP_SIGNATURE); /* Ensuring all pages have been split into one fragment initially: * page_pool_set_pp_info() is only called once for every page when it @@ -464,10 +462,8 @@ static void page_pool_set_pp_info(struct page_pool *pool, netmem_ref netmem) static void page_pool_clear_pp_info(netmem_ref netmem) { - struct page *page = netmem_to_page(netmem); - - page->pp_magic = 0; - page->pp = NULL; + netmem_clear_pp_magic(netmem); + netmem_set_pp(netmem, NULL); } static struct page *__page_pool_alloc_page_order(struct page_pool *pool, @@ -695,8 +691,9 @@ static bool page_pool_recycle_in_cache(netmem_ref netmem, static bool __page_pool_page_can_be_recycled(netmem_ref netmem) { - return page_ref_count(netmem_to_page(netmem)) == 1 && - !page_is_pfmemalloc(netmem_to_page(netmem)); + return netmem_is_net_iov(netmem) || + (page_ref_count(netmem_to_page(netmem)) == 1 && + !page_is_pfmemalloc(netmem_to_page(netmem))); } /* If the page refcnt == 1, this will try to recycle the page. @@ -718,7 +715,7 @@ __page_pool_put_page(struct page_pool *pool, netmem_ref netmem, * refcnt == 1 means page_pool owns page, and can recycle it. * * page is NOT reusable when allocated when system is under - * some pressure. (page_is_pfmemalloc) + * some pressure. (page_pool_page_is_pfmemalloc) */ if (likely(__page_pool_page_can_be_recycled(netmem))) { /* Read barrier done in page_ref_count / READ_ONCE */ @@ -734,6 +731,7 @@ __page_pool_put_page(struct page_pool *pool, netmem_ref netmem, /* Page found as candidate for recycling */ return netmem; } + /* Fallback/non-XDP mode: API user have elevated refcnt. * * Many drivers split up the page into fragments, and some @@ -928,7 +926,7 @@ static void page_pool_empty_ring(struct page_pool *pool) /* Empty recycle ring */ while ((netmem = (__force netmem_ref)ptr_ring_consume_bh(&pool->ring))) { /* Verify the refcnt invariant of cached pages */ - if (!(page_ref_count(netmem_to_page(netmem)) == 1)) + if (!(netmem_ref_count(netmem) == 1)) pr_crit("%s() page_pool refcnt %d violation\n", __func__, netmem_ref_count(netmem)); diff --git a/net/core/skbuff.c b/net/core/skbuff.c index dc6b1f6435e2..753d61680d69 100644 --- a/net/core/skbuff.c +++ b/net/core/skbuff.c @@ -906,9 +906,9 @@ static void skb_clone_fraglist(struct sk_buff *skb) skb_get(list); } -static bool is_pp_page(struct page *page) +static bool is_pp_netmem(netmem_ref netmem) { - return (page->pp_magic & ~0x3UL) == PP_SIGNATURE; + return (netmem_get_pp_magic(netmem) & ~0x3UL) == PP_SIGNATURE; } int skb_pp_cow_data(struct page_pool *pool, struct sk_buff **pskb, @@ -1006,11 +1006,10 @@ EXPORT_SYMBOL(skb_cow_data_for_xdp); #if IS_ENABLED(CONFIG_PAGE_POOL) bool napi_pp_put_page(netmem_ref netmem, bool napi_safe) { - struct page *page = netmem_to_page(netmem); bool allow_direct = false; struct page_pool *pp; - page = compound_head(page); + netmem = netmem_compound_head(netmem); /* page->pp_magic is OR'ed with PP_SIGNATURE after the allocation * in order to preserve any existing bits, such as bit 0 for the @@ -1019,10 +1018,10 @@ bool napi_pp_put_page(netmem_ref netmem, bool napi_safe) * and page_is_pfmemalloc() is checked in __page_pool_put_page() * to avoid recycling the pfmemalloc page. */ - if (unlikely(!is_pp_page(page))) + if (unlikely(!is_pp_netmem(netmem))) return false; - pp = page->pp; + pp = netmem_get_pp(netmem); /* Allow direct recycle if we have reasons to believe that we are * in the same context as the consumer would run, so there's @@ -1043,7 +1042,7 @@ bool napi_pp_put_page(netmem_ref netmem, bool napi_safe) * The page will be returned to the pool here regardless of the * 'flipped' fragment being in use or not. */ - page_pool_put_full_netmem(pp, page_to_netmem(page), allow_direct); + page_pool_put_full_netmem(pp, netmem, allow_direct); return true; } @@ -1070,7 +1069,7 @@ static bool skb_pp_recycle(struct sk_buff *skb, void *data, bool napi_safe) static int skb_pp_frag_ref(struct sk_buff *skb) { struct skb_shared_info *shinfo; - struct page *head_page; + netmem_ref head_netmem; int i; if (!skb->pp_recycle) @@ -1079,11 +1078,11 @@ static int skb_pp_frag_ref(struct sk_buff *skb) shinfo = skb_shinfo(skb); for (i = 0; i < shinfo->nr_frags; i++) { - head_page = compound_head(skb_frag_page(&shinfo->frags[i])); - if (likely(is_pp_page(head_page))) - page_pool_ref_page(head_page); + head_netmem = netmem_compound_head(shinfo->frags[i].netmem); + if (likely(is_pp_netmem(head_netmem))) + page_pool_ref_netmem(head_netmem); else - page_ref_inc(head_page); + page_ref_inc(netmem_to_page(head_netmem)); } return 0; }
Convert netmem to be a union of struct page and struct netmem. Overload the LSB of struct netmem* to indicate that it's a net_iov, otherwise it's a page. Currently these entries in struct page are rented by the page_pool and used exclusively by the net stack: struct { unsigned long pp_magic; struct page_pool *pp; unsigned long _pp_mapping_pad; unsigned long dma_addr; atomic_long_t pp_ref_count; }; Mirror these (and only these) entries into struct net_iov and implement netmem helpers that can access these common fields regardless of whether the underlying type is page or net_iov. Implement checks for net_iov in netmem helpers which delegate to mm APIs, to ensure net_iov are never passed to the mm stack. Signed-off-by: Mina Almasry <almasrymina@google.com> --- v7: - Remove static_branch_unlikely from netmem_to_net_iov(). We're getting better results from the fast path in bench_page_pool_simple tests without the static_branch_unlikely, and the addition of static_branch_unlikely doesn't improve performance of devmem TCP. Additionally only check netmem_to_net_iov() if CONFIG_DMA_SHARED_BUFFER is enabled, otherwise dmabuf net_iovs cannot exist anyway. net-next base: 8 cycle fast path. with static_branch_unlikely: 10 cycle fast path. without static_branch_unlikely: 9 cycle fast path. CONFIG_DMA_SHARED_BUFFER disabled: 8 cycle fast path as baseline. Performance of devmem TCP is at 95% line rate is regardless of static_branch_unlikely or not. v6: - Rebased on top of the merged netmem_ref type. - Rebased on top of the merged skb_pp_frag_ref() changes. v5: - Use netmem instead of page* with LSB set. - Use pp_ref_count for refcounting net_iov. - Removed many of the custom checks for netmem. v1: - Disable fragmentation support for iov properly. - fix napi_pp_put_page() path (Yunsheng). - Use pp_frag_count for devmem refcounting. Cc: linux-mm@kvack.org Cc: Matthew Wilcox <willy@infradead.org> --- include/net/netmem.h | 141 ++++++++++++++++++++++++++++++-- include/net/page_pool/helpers.h | 25 +++--- net/core/devmem.c | 3 + net/core/page_pool.c | 26 +++--- net/core/skbuff.c | 23 +++--- 5 files changed, 172 insertions(+), 46 deletions(-)