On Thu, 29 Jun 2023 20:02:21 +0800 Yunsheng Lin wrote:
> -#include <linux/dma-direction.h>
> +#include <linux/dma-mapping.h>
And the include is still here, too, eh..
On 2023/7/8 8:01, Jakub Kicinski wrote:
> On Thu, 29 Jun 2023 20:02:21 +0800 Yunsheng Lin wrote:
>> -#include <linux/dma-direction.h>
>> +#include <linux/dma-mapping.h>
>
> And the include is still here, too, eh..
In V4, it has:
--- a/include/net/page_pool.h
+++ b/include/net/page_pool.h
@@ -33,6 +33,7 @@
#include <linux/mm.h> /* Needed by ptr_ring */
#include <linux/ptr_ring.h>
#include <linux/dma-direction.h>
+#include <linux/dma-mapping.h>
As dma_get_cache_alignment() defined in dma-mapping.h is used
here, so we need to include dma-mapping.h.
I though the agreement is that this patch only remove the
"#include <linux/dma-direction.h>" as we dma-mapping.h has included
dma-direction.h.
And Alexander will work on excluding page_pool.h from skbuff.h
https://lore.kernel.org/all/[email protected]/
Did I miss something obvious here？ Or there is better way to do it
than the method discussed in the above thread?
>
On Sun, 9 Jul 2023 20:54:12 +0800 Yunsheng Lin wrote:
> > And the include is still here, too, eh..
>
> In V4, it has:
>
> --- a/include/net/page_pool.h
> +++ b/include/net/page_pool.h
> @@ -33,6 +33,7 @@
> #include <linux/mm.h> /* Needed by ptr_ring */
> #include <linux/ptr_ring.h>
> #include <linux/dma-direction.h>
> +#include <linux/dma-mapping.h>
>
> As dma_get_cache_alignment() defined in dma-mapping.h is used
> here, so we need to include dma-mapping.h.
>
> I though the agreement is that this patch only remove the
> "#include <linux/dma-direction.h>" as we dma-mapping.h has included
> dma-direction.h.
>
> And Alexander will work on excluding page_pool.h from skbuff.h
> https://lore.kernel.org/all/[email protected]/
>
> Did I miss something obvious here？ Or there is better way to do it
> than the method discussed in the above thread?
We're adding a ton of static inline functions to what is a fairly core
header for networking, that's what re-triggered by complaint:
include/net/page_pool.h | 179 ++++++++++++++----
Maybe we should revisit the idea of creating a new header file for
inline helpers... Olek, WDYT?
From: Jakub Kicinski <[email protected]>
Date: Mon, 10 Jul 2023 11:38:41 -0700
> On Sun, 9 Jul 2023 20:54:12 +0800 Yunsheng Lin wrote:
>>> And the include is still here, too, eh..
>>
>> In V4, it has:
>>
>> --- a/include/net/page_pool.h
>> +++ b/include/net/page_pool.h
>> @@ -33,6 +33,7 @@
>> #include <linux/mm.h> /* Needed by ptr_ring */
>> #include <linux/ptr_ring.h>
>> #include <linux/dma-direction.h>
>> +#include <linux/dma-mapping.h>
>>
>> As dma_get_cache_alignment() defined in dma-mapping.h is used
>> here, so we need to include dma-mapping.h.
>>
>> I though the agreement is that this patch only remove the
>> "#include <linux/dma-direction.h>" as we dma-mapping.h has included
>> dma-direction.h.
>>
>> And Alexander will work on excluding page_pool.h from skbuff.h
>> https://lore.kernel.org/all/[email protected]/
>>
>> Did I miss something obvious here？ Or there is better way to do it
>> than the method discussed in the above thread?
>
> We're adding a ton of static inline functions to what is a fairly core
> header for networking, that's what re-triggered by complaint:
>
> include/net/page_pool.h | 179 ++++++++++++++----
>
> Maybe we should revisit the idea of creating a new header file for
> inline helpers... Olek, WDYT?
I'm fine with that, although ain't really able to work on this myself
now :s (BTW I almost finished Netlink bigints, just some more libie/IAVF
crap).
It just needs to be carefully designed, because if we want move ALL the
inlines to a new header, we may end up including 2 PP's headers in each
file. That's why I'd prefer "core/driver" separation. Let's say skbuff.c
doesn't need page_pool_create(), page_pool_alloc(), and so on, while
drivers don't need some of its internal functions.
OTOH after my patch it's included in only around 20-30 files on
allmodconfig. That is literally nothing comparing to e.g. kernel.h
(w/includes) :D
Thanks,
Olek
On Tue, 11 Jul 2023 12:59:00 +0200 Alexander Lobakin wrote:
> I'm fine with that, although ain't really able to work on this myself
> now :s (BTW I almost finished Netlink bigints, just some more libie/IAVF
> crap).
FWIW I was thinking about the bigints recently, and from ynl
perspective I think we may want two flavors :( One which is at
most the length of platform's long long, and another which is
always a bigint. The latter will be more work for user space
to handle, so given 99% of use cases don't need more than 64b
we should make its life easier?
> It just needs to be carefully designed, because if we want move ALL the
> inlines to a new header, we may end up including 2 PP's headers in each
> file. That's why I'd prefer "core/driver" separation. Let's say skbuff.c
> doesn't need page_pool_create(), page_pool_alloc(), and so on, while
> drivers don't need some of its internal functions.
> OTOH after my patch it's included in only around 20-30 files on
> allmodconfig. That is literally nothing comparing to e.g. kernel.h
> (w/includes) :D
Well, once you have to rebuilding 100+ files it gets pretty hard to
clean things up ;)
I think I described the preferred setup, previously:
$path/page_pool.h:
#include <$path/page_pool/types.h>
#include <$path/page_pool/helpers.h>
$path/page_pool/types.h - has types
$path/page_pool/helpers.h - has all the inlines
C sources can include $path/page_pool.h, headers should generally only
include $path/page_pool/types.h.
From: Jakub Kicinski <[email protected]>
Date: Tue, 11 Jul 2023 09:37:05 -0700
> On Tue, 11 Jul 2023 12:59:00 +0200 Alexander Lobakin wrote:
>> I'm fine with that, although ain't really able to work on this myself
>> now :s (BTW I almost finished Netlink bigints, just some more libie/IAVF
>> crap).
>
> FWIW I was thinking about the bigints recently, and from ynl
> perspective I think we may want two flavors :( One which is at
> most the length of platform's long long, and another which is
(not sure we shouldn't split a separate thread off this one at this
point :D)
`long long` or `long`? `long long` is always 64-bit unless I'm missing
something. On my 32-bit MIPS they were :D
If `long long`, what's the point then if we have %NLA_U64 and would
still have to add dumb padding attrs? :D I thought the idea was to carry
64+ bits encapsulated in 32-bit primitives.
> always a bigint. The latter will be more work for user space
> to handle, so given 99% of use cases don't need more than 64b
> we should make its life easier?
>
>> It just needs to be carefully designed, because if we want move ALL the
>> inlines to a new header, we may end up including 2 PP's headers in each
>> file. That's why I'd prefer "core/driver" separation. Let's say skbuff.c
>> doesn't need page_pool_create(), page_pool_alloc(), and so on, while
>> drivers don't need some of its internal functions.
>> OTOH after my patch it's included in only around 20-30 files on
>> allmodconfig. That is literally nothing comparing to e.g. kernel.h
>> (w/includes) :D
>
> Well, once you have to rebuilding 100+ files it gets pretty hard to
> clean things up ;)
>
> I think I described the preferred setup, previously:
>
> $path/page_pool.h:
>
> #include <$path/page_pool/types.h>
> #include <$path/page_pool/helpers.h>
>
> $path/page_pool/types.h - has types
> $path/page_pool/helpers.h - has all the inlines
>
> C sources can include $path/page_pool.h, headers should generally only
> include $path/page_pool/types.h.
Aaah okay, I did read it backwards ._. Moreover, generic stack barely
uses PP's inlines, it needs externals mostly.
Thanks,
Olek
On Tue, 11 Jul 2023 18:59:51 +0200 Alexander Lobakin wrote:
> From: Jakub Kicinski <[email protected]>
> Date: Tue, 11 Jul 2023 09:37:05 -0700
>
> > On Tue, 11 Jul 2023 12:59:00 +0200 Alexander Lobakin wrote:
> >> I'm fine with that, although ain't really able to work on this myself
> >> now :s (BTW I almost finished Netlink bigints, just some more libie/IAVF
> >> crap).
> >
> > FWIW I was thinking about the bigints recently, and from ynl
> > perspective I think we may want two flavors :( One which is at
> > most the length of platform's long long, and another which is
>
> `long long` or `long`? `long long` is always 64-bit unless I'm missing
> something. On my 32-bit MIPS they were :D
> If `long long`, what's the point then if we have %NLA_U64 and would
> still have to add dumb padding attrs? :D I thought the idea was to carry
> 64+ bits encapsulated in 32-bit primitives.
Sorry I confused things. Keep in mind we're only talking about what
the generated YNL code ends up looking like, not the "wire" format.
So we still "transport" things as multiple 32b chunks at netlink level.
No padding.
The question is how to render the C / C++ code on the YNL side (or
any practical library). Are we storing all those values as bigints and
require users to coerce them to a more natural type on each access?
That'd defeat the goal of the new int type becoming the default /
"don't overthink the sizing" type.
If we have a subtype with a max size of 64b, it can be 32b or 64b on
the wire, as needed, but user space can feel assured that u64 will
always be able to store the result.
The long long is my misguided attempt to be platform dependent.
I think a better way of putting it would actually be 2 * sizeof(long).
That way we can use u128 as max, which seems to only be defined on 64b
platforms. But that's just a random thought, I'm not sure how useful
it would be.
Perhaps we need two types, one "basic" which tops out at 64b and one
"really bigint" which can be used as bitmaps as well?