diff mbox series

[bpf-next,2/3] xdp: xdp_umem: replace kmap on vmap for umem map

Message ID 20190813102318.5521-3-ivan.khoronzhuk@linaro.org
State New
Headers show
Series xdpsock: allow mmap2 usage for 32bits | expand

Commit Message

Ivan Khoronzhuk Aug. 13, 2019, 10:23 a.m. UTC 
For 64-bit there is no reason to use vmap/vunmap, so use page_address
as it was initially. For 32 bits, in some apps, like in samples
xdpsock_user.c when number of pgs in use is quite big, the kmap
memory can be not enough, despite on this, kmap looks like is
deprecated in such cases as it can block and should be used rather
for dynamic mm.

Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org>

---
 net/xdp/xdp_umem.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

-- 
2.17.1

Comments

Jonathan Lemon Aug. 13, 2019, 5:42 p.m. UTC | #1 
On 13 Aug 2019, at 3:23, Ivan Khoronzhuk wrote:

> For 64-bit there is no reason to use vmap/vunmap, so use page_address

> as it was initially. For 32 bits, in some apps, like in samples

> xdpsock_user.c when number of pgs in use is quite big, the kmap

> memory can be not enough, despite on this, kmap looks like is

> deprecated in such cases as it can block and should be used rather

> for dynamic mm.

>

> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org>


Seems a bit overkill - if not high memory, kmap() falls back
to just page_address(), unlike vmap().
-- 
Jonathan

> ---

>  net/xdp/xdp_umem.c | 16 ++++++++++++----

>  1 file changed, 12 insertions(+), 4 deletions(-)

>

> diff --git a/net/xdp/xdp_umem.c b/net/xdp/xdp_umem.c

> index a0607969f8c0..907c9019fe21 100644

> --- a/net/xdp/xdp_umem.c

> +++ b/net/xdp/xdp_umem.c

> @@ -14,7 +14,7 @@

>  #include <linux/netdevice.h>

>  #include <linux/rtnetlink.h>

>  #include <linux/idr.h>

> -#include <linux/highmem.h>

> +#include <linux/vmalloc.h>

>

>  #include "xdp_umem.h"

>  #include "xsk_queue.h"

> @@ -167,10 +167,12 @@ void xdp_umem_clear_dev(struct xdp_umem *umem)

>

>  static void xdp_umem_unmap_pages(struct xdp_umem *umem)

>  {

> +#if BITS_PER_LONG == 32

>  	unsigned int i;

>

>  	for (i = 0; i < umem->npgs; i++)

> -		kunmap(umem->pgs[i]);

> +		vunmap(umem->pages[i].addr);

> +#endif

>  }

>

>  static void xdp_umem_unpin_pages(struct xdp_umem *umem)

> @@ -378,8 +380,14 @@ static int xdp_umem_reg(struct xdp_umem *umem, 

> struct xdp_umem_reg *mr)

>  		goto out_account;

>  	}

>

> -	for (i = 0; i < umem->npgs; i++)

> -		umem->pages[i].addr = kmap(umem->pgs[i]);

> +	for (i = 0; i < umem->npgs; i++) {

> +#if BITS_PER_LONG == 32

> +		umem->pages[i].addr = vmap(&umem->pgs[i], 1, VM_MAP,

> +					   PAGE_KERNEL);

> +#else

> +		umem->pages[i].addr = page_address(umem->pgs[i]);

> +#endif

> +	}

>

>  	return 0;

>

> -- 

> 2.17.1
Ivan Khoronzhuk Aug. 13, 2019, 6:30 p.m. UTC | #2 
On Tue, Aug 13, 2019 at 10:42:18AM -0700, Jonathan Lemon wrote:
>

>

>On 13 Aug 2019, at 3:23, Ivan Khoronzhuk wrote:

>

>>For 64-bit there is no reason to use vmap/vunmap, so use page_address

>>as it was initially. For 32 bits, in some apps, like in samples

>>xdpsock_user.c when number of pgs in use is quite big, the kmap

>>memory can be not enough, despite on this, kmap looks like is

>>deprecated in such cases as it can block and should be used rather

>>for dynamic mm.

>>

>>Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org>

>

>Seems a bit overkill - if not high memory, kmap() falls back

>to just page_address(), unlike vmap().


>-- Jonathan


So, as kmap has limitation... if I correctly understood, you propose
to avoid macros and do smth like kmap:

	void *addr;
	if (!PageHighMem(&umem->pgs[i]))
		addr =  page_address(page);
	else
		addr = vmap(&umem->pgs[i], 1, VM_MAP, PAGE_KERNEL);

	umem->pages[i].addr = addr;

and while unmap

	if (!PageHighMem(&umem->pgs[i]))
		vunmap(umem->pages[i].addr);

I can try it, and add this in v2 if no objection.

>

>>---

>> net/xdp/xdp_umem.c | 16 ++++++++++++----

>> 1 file changed, 12 insertions(+), 4 deletions(-)

>>

>>diff --git a/net/xdp/xdp_umem.c b/net/xdp/xdp_umem.c

>>index a0607969f8c0..907c9019fe21 100644

>>--- a/net/xdp/xdp_umem.c

>>+++ b/net/xdp/xdp_umem.c

>>@@ -14,7 +14,7 @@

>> #include <linux/netdevice.h>

>> #include <linux/rtnetlink.h>

>> #include <linux/idr.h>

>>-#include <linux/highmem.h>

>>+#include <linux/vmalloc.h>

>>

>> #include "xdp_umem.h"

>> #include "xsk_queue.h"

>>@@ -167,10 +167,12 @@ void xdp_umem_clear_dev(struct xdp_umem *umem)

>>

>> static void xdp_umem_unmap_pages(struct xdp_umem *umem)

>> {

>>+#if BITS_PER_LONG == 32

>> 	unsigned int i;

>>

>> 	for (i = 0; i < umem->npgs; i++)

>>-		kunmap(umem->pgs[i]);

>>+		vunmap(umem->pages[i].addr);

>>+#endif

>> }

>>

>> static void xdp_umem_unpin_pages(struct xdp_umem *umem)

>>@@ -378,8 +380,14 @@ static int xdp_umem_reg(struct xdp_umem *umem, 

>>struct xdp_umem_reg *mr)

>> 		goto out_account;

>> 	}

>>

>>-	for (i = 0; i < umem->npgs; i++)

>>-		umem->pages[i].addr = kmap(umem->pgs[i]);

>>+	for (i = 0; i < umem->npgs; i++) {

>>+#if BITS_PER_LONG == 32

>>+		umem->pages[i].addr = vmap(&umem->pgs[i], 1, VM_MAP,

>>+					   PAGE_KERNEL);

>>+#else

>>+		umem->pages[i].addr = page_address(umem->pgs[i]);

>>+#endif

>>+	}

>>

>> 	return 0;

>>

>>-- 

>>2.17.1


-- 
Regards,
Ivan Khoronzhuk
Jonathan Lemon Aug. 13, 2019, 6:33 p.m. UTC | #3 
On 13 Aug 2019, at 11:30, Ivan Khoronzhuk wrote:

> On Tue, Aug 13, 2019 at 10:42:18AM -0700, Jonathan Lemon wrote:

>>

>>

>> On 13 Aug 2019, at 3:23, Ivan Khoronzhuk wrote:

>>

>>> For 64-bit there is no reason to use vmap/vunmap, so use 

>>> page_address

>>> as it was initially. For 32 bits, in some apps, like in samples

>>> xdpsock_user.c when number of pgs in use is quite big, the kmap

>>> memory can be not enough, despite on this, kmap looks like is

>>> deprecated in such cases as it can block and should be used rather

>>> for dynamic mm.

>>>

>>> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org>

>>

>> Seems a bit overkill - if not high memory, kmap() falls back

>> to just page_address(), unlike vmap().

>

>> -- Jonathan

>

> So, as kmap has limitation... if I correctly understood, you propose

> to avoid macros and do smth like kmap:

>

> 	void *addr;

> 	if (!PageHighMem(&umem->pgs[i]))

> 		addr =  page_address(page);

> 	else

> 		addr = vmap(&umem->pgs[i], 1, VM_MAP, PAGE_KERNEL);

>

> 	umem->pages[i].addr = addr;

>

> and while unmap

>

> 	if (!PageHighMem(&umem->pgs[i]))

> 		vunmap(umem->pages[i].addr);

>

> I can try it, and add this in v2 if no objection.


Seems like a reasonable compromise to me.
-- 
Jonathan


>

>>

>>> ---

>>> net/xdp/xdp_umem.c | 16 ++++++++++++----

>>> 1 file changed, 12 insertions(+), 4 deletions(-)

>>>

>>> diff --git a/net/xdp/xdp_umem.c b/net/xdp/xdp_umem.c

>>> index a0607969f8c0..907c9019fe21 100644

>>> --- a/net/xdp/xdp_umem.c

>>> +++ b/net/xdp/xdp_umem.c

>>> @@ -14,7 +14,7 @@

>>> #include <linux/netdevice.h>

>>> #include <linux/rtnetlink.h>

>>> #include <linux/idr.h>

>>> -#include <linux/highmem.h>

>>> +#include <linux/vmalloc.h>

>>>

>>> #include "xdp_umem.h"

>>> #include "xsk_queue.h"

>>> @@ -167,10 +167,12 @@ void xdp_umem_clear_dev(struct xdp_umem *umem)

>>>

>>> static void xdp_umem_unmap_pages(struct xdp_umem *umem)

>>> {

>>> +#if BITS_PER_LONG == 32

>>> 	unsigned int i;

>>>

>>> 	for (i = 0; i < umem->npgs; i++)

>>> -		kunmap(umem->pgs[i]);

>>> +		vunmap(umem->pages[i].addr);

>>> +#endif

>>> }

>>>

>>> static void xdp_umem_unpin_pages(struct xdp_umem *umem)

>>> @@ -378,8 +380,14 @@ static int xdp_umem_reg(struct xdp_umem *umem, 

>>> struct xdp_umem_reg *mr)

>>> 		goto out_account;

>>> 	}

>>>

>>> -	for (i = 0; i < umem->npgs; i++)

>>> -		umem->pages[i].addr = kmap(umem->pgs[i]);

>>> +	for (i = 0; i < umem->npgs; i++) {

>>> +#if BITS_PER_LONG == 32

>>> +		umem->pages[i].addr = vmap(&umem->pgs[i], 1, VM_MAP,

>>> +					   PAGE_KERNEL);

>>> +#else

>>> +		umem->pages[i].addr = page_address(umem->pgs[i]);

>>> +#endif

>>> +	}

>>>

>>> 	return 0;

>>>

>>> -- 

>>> 2.17.1

>

> -- 

> Regards,

> Ivan Khoronzhuk
diff mbox series

Patch

diff --git a/net/xdp/xdp_umem.c b/net/xdp/xdp_umem.c
index a0607969f8c0..907c9019fe21 100644
--- a/net/xdp/xdp_umem.c
+++ b/net/xdp/xdp_umem.c
@@ -14,7 +14,7 @@ 
 #include <linux/netdevice.h>
 #include <linux/rtnetlink.h>
 #include <linux/idr.h>
-#include <linux/highmem.h>
+#include <linux/vmalloc.h>
 
 #include "xdp_umem.h"
 #include "xsk_queue.h"
@@ -167,10 +167,12 @@  void xdp_umem_clear_dev(struct xdp_umem *umem)
 
 static void xdp_umem_unmap_pages(struct xdp_umem *umem)
 {
+#if BITS_PER_LONG == 32
 	unsigned int i;
 
 	for (i = 0; i < umem->npgs; i++)
-		kunmap(umem->pgs[i]);
+		vunmap(umem->pages[i].addr);
+#endif
 }
 
 static void xdp_umem_unpin_pages(struct xdp_umem *umem)
@@ -378,8 +380,14 @@  static int xdp_umem_reg(struct xdp_umem *umem, struct xdp_umem_reg *mr)
 		goto out_account;
 	}
 
-	for (i = 0; i < umem->npgs; i++)
-		umem->pages[i].addr = kmap(umem->pgs[i]);
+	for (i = 0; i < umem->npgs; i++) {
+#if BITS_PER_LONG == 32
+		umem->pages[i].addr = vmap(&umem->pgs[i], 1, VM_MAP,
+					   PAGE_KERNEL);
+#else
+		umem->pages[i].addr = page_address(umem->pgs[i]);
+#endif
+	}
 
 	return 0;