Re: [PATCH v4 bpf-next 0/8] bpf_prog_pack followup

Re: [PATCH v4 bpf-next 0/8] bpf_prog_pack followup

[patchwork-bot+netdevbpf]

Hello:

This series was applied to bpf/bpf-next.git (master)
by Daniel Borkmann <daniel@iogearbox.net>:

On Fri, 20 May 2022 16:57:50 -0700 you wrote:
> Changes v3 => v4:
> 1. Shorten CC list on 4/8, so it is not dropped by the mail list.
> 
> Changes v2 => v3:
> 1. Fix issues reported by kernel test robot <lkp@intel.com>.
> 
> Changes v1 => v2:
> 1. Add WARN to set_vm_flush_reset_perms() on huge pages. (Rick Edgecombe)
> 2. Simplify select_bpf_prog_pack_size. (Rick Edgecombe)
> 
> [...]

Here is the summary with links:
  - [v4,bpf-next,1/8] bpf: fill new bpf_prog_pack with illegal instructions
    https://git.kernel.org/bpf/bpf-next/c/d88bb5eed04c
  - [v4,bpf-next,2/8] x86/alternative: introduce text_poke_set
    https://git.kernel.org/bpf/bpf-next/c/aadd1b678ebe
  - [v4,bpf-next,3/8] bpf: introduce bpf_arch_text_invalidate for bpf_prog_pack
    https://git.kernel.org/bpf/bpf-next/c/fe736565efb7
  - [v4,bpf-next,4/8] module: introduce module_alloc_huge
    (no matching commit)
  - [v4,bpf-next,5/8] bpf: use module_alloc_huge for bpf_prog_pack
    (no matching commit)
  - [v4,bpf-next,6/8] vmalloc: WARN for set_vm_flush_reset_perms() on huge pages
    (no matching commit)
  - [v4,bpf-next,7/8] vmalloc: introduce huge_vmalloc_supported
    (no matching commit)
  - [v4,bpf-next,8/8] bpf: simplify select_bpf_prog_pack_size
    (no matching commit)

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Re: [PATCH v4 bpf-next 3/8] bpf: introduce bpf_arch_text_invalidate for bpf_prog_pack

[Mike Rapoport]

On Fri, May 20, 2022 at 04:57:53PM -0700, Song Liu wrote:
> Introduce bpf_arch_text_invalidate and use it to fill unused part of the
> bpf_prog_pack with illegal instructions when a BPF program is freed.
> 
> Signed-off-by: Song Liu <song@kernel.org>
> ---
>  arch/x86/net/bpf_jit_comp.c | 5 +++++
>  include/linux/bpf.h         | 1 +
>  kernel/bpf/core.c           | 8 ++++++++
>  3 files changed, 14 insertions(+)
> 
> diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
> index a2b6d197c226..f298b18a9a3d 100644
> --- a/arch/x86/net/bpf_jit_comp.c
> +++ b/arch/x86/net/bpf_jit_comp.c
> @@ -228,6 +228,11 @@ static void jit_fill_hole(void *area, unsigned int size)
>  	memset(area, 0xcc, size);
>  }
>  
> +int bpf_arch_text_invalidate(void *dst, size_t len)
> +{
> +	return IS_ERR_OR_NULL(text_poke_set(dst, 0xcc, len));
> +}
> +
>  struct jit_context {
>  	int cleanup_addr; /* Epilogue code offset */
>  
> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> index cc4d5e394031..a9b1875212f6 100644
> --- a/include/linux/bpf.h
> +++ b/include/linux/bpf.h
> @@ -2365,6 +2365,7 @@ int bpf_arch_text_poke(void *ip, enum bpf_text_poke_type t,
>  		       void *addr1, void *addr2);
>  
>  void *bpf_arch_text_copy(void *dst, void *src, size_t len);
> +int bpf_arch_text_invalidate(void *dst, size_t len);
>  
>  struct btf_id_set;
>  bool btf_id_set_contains(const struct btf_id_set *set, u32 id);
> diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
> index 2d0c9d4696ad..cacd8684c3c4 100644
> --- a/kernel/bpf/core.c
> +++ b/kernel/bpf/core.c
> @@ -968,6 +968,9 @@ static void bpf_prog_pack_free(struct bpf_binary_header *hdr)
>  	nbits = BPF_PROG_SIZE_TO_NBITS(hdr->size);
>  	pos = ((unsigned long)hdr - (unsigned long)pack_ptr) >> BPF_PROG_CHUNK_SHIFT;
>  
> +	WARN_ONCE(bpf_arch_text_invalidate(hdr, hdr->size),
> +		  "bpf_prog_pack bug: missing bpf_arch_text_invalidate?\n");

Why is this a WARN?

What happens if bpf_arch_text_invalidate() is implemented but returns an error?

> +
>  	bitmap_clear(pack->bitmap, pos, nbits);
>  	if (bitmap_find_next_zero_area(pack->bitmap, bpf_prog_chunk_count(), 0,
>  				       bpf_prog_chunk_count(), 0) == 0) {
> @@ -2740,6 +2743,11 @@ void * __weak bpf_arch_text_copy(void *dst, void *src, size_t len)
>  	return ERR_PTR(-ENOTSUPP);
>  }
>  
> +int __weak bpf_arch_text_invalidate(void *dst, size_t len)
> +{
> +	return -ENOTSUPP;
> +}
> +
>  DEFINE_STATIC_KEY_FALSE(bpf_stats_enabled_key);
>  EXPORT_SYMBOL(bpf_stats_enabled_key);
>  
> -- 
> 2.30.2
> 
> 

-- 
Sincerely yours,
Mike.

Re: [PATCH v4 bpf-next 5/8] bpf: use module_alloc_huge for bpf_prog_pack

[Mike Rapoport]

On Fri, May 20, 2022 at 04:57:55PM -0700, Song Liu wrote:
> Use module_alloc_huge for bpf_prog_pack so that BPF programs sit on
> PMD_SIZE pages. This benefits system performance by reducing iTLB miss
> rate. Benchmark of a real web service workload shows this change gives
> another ~0.2% performance boost on top of PAGE_SIZE bpf_prog_pack
> (which improve system throughput by ~0.5%).
> 
> Also, remove set_vm_flush_reset_perms() from alloc_new_pack() and use
> set_memory_[nx|rw] in bpf_prog_pack_free(). This is because
> VM_FLUSH_RESET_PERMS does not work with huge pages yet. [1]
> 
> [1] https://lore.kernel.org/bpf/aeeeaf0b7ec63fdba55d4834d2f524d8bf05b71b.camel@intel.com/
> Suggested-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
> Signed-off-by: Song Liu <song@kernel.org>
> ---
>  kernel/bpf/core.c | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
> index cacd8684c3c4..b64d91fcb0ba 100644
> --- a/kernel/bpf/core.c
> +++ b/kernel/bpf/core.c
> @@ -857,7 +857,7 @@ static size_t select_bpf_prog_pack_size(void)
>  	void *ptr;
>  
>  	size = BPF_HPAGE_SIZE * num_online_nodes();
> -	ptr = module_alloc(size);
> +	ptr = module_alloc_huge(size);
>  
>  	/* Test whether we can get huge pages. If not just use PAGE_SIZE
>  	 * packs.
> @@ -881,7 +881,7 @@ static struct bpf_prog_pack *alloc_new_pack(bpf_jit_fill_hole_t bpf_fill_ill_ins
>  		       GFP_KERNEL);
>  	if (!pack)
>  		return NULL;
> -	pack->ptr = module_alloc(bpf_prog_pack_size);
> +	pack->ptr = module_alloc_huge(bpf_prog_pack_size);
>  	if (!pack->ptr) {
>  		kfree(pack);
>  		return NULL;
> @@ -890,7 +890,6 @@ static struct bpf_prog_pack *alloc_new_pack(bpf_jit_fill_hole_t bpf_fill_ill_ins
>  	bitmap_zero(pack->bitmap, bpf_prog_pack_size / BPF_PROG_CHUNK_SIZE);
>  	list_add_tail(&pack->list, &pack_list);
>  
> -	set_vm_flush_reset_perms(pack->ptr);
>  	set_memory_ro((unsigned long)pack->ptr, bpf_prog_pack_size / PAGE_SIZE);
>  	set_memory_x((unsigned long)pack->ptr, bpf_prog_pack_size / PAGE_SIZE);
>  	return pack;
> @@ -909,10 +908,9 @@ static void *bpf_prog_pack_alloc(u32 size, bpf_jit_fill_hole_t bpf_fill_ill_insn
>  
>  	if (size > bpf_prog_pack_size) {
>  		size = round_up(size, PAGE_SIZE);
> -		ptr = module_alloc(size);
> +		ptr = module_alloc_huge(size);
>  		if (ptr) {
>  			bpf_fill_ill_insns(ptr, size);
> -			set_vm_flush_reset_perms(ptr);
>  			set_memory_ro((unsigned long)ptr, size / PAGE_SIZE);
>  			set_memory_x((unsigned long)ptr, size / PAGE_SIZE);
>  		}
> @@ -949,6 +947,8 @@ static void bpf_prog_pack_free(struct bpf_binary_header *hdr)
>  
>  	mutex_lock(&pack_mutex);
>  	if (hdr->size > bpf_prog_pack_size) {
> +		set_memory_nx((unsigned long)hdr, hdr->size / PAGE_SIZE);
> +		set_memory_rw((unsigned long)hdr, hdr->size / PAGE_SIZE);

set_memory_{nx,rw} can fail. Please take care of error handling.

>  		module_memfree(hdr);
>  		goto out;
>  	}
> @@ -975,6 +975,8 @@ static void bpf_prog_pack_free(struct bpf_binary_header *hdr)
>  	if (bitmap_find_next_zero_area(pack->bitmap, bpf_prog_chunk_count(), 0,
>  				       bpf_prog_chunk_count(), 0) == 0) {
>  		list_del(&pack->list);
> +		set_memory_nx((unsigned long)pack->ptr, bpf_prog_pack_size / PAGE_SIZE);
> +		set_memory_rw((unsigned long)pack->ptr, bpf_prog_pack_size / PAGE_SIZE);

ditto.

>  		module_memfree(pack->ptr);
>  		kfree(pack);
>  	}
> -- 
> 2.30.2
> 
> 

-- 
Sincerely yours,
Mike.

Re: [PATCH v4 bpf-next 5/8] bpf: use module_alloc_huge for bpf_prog_pack

[Edgecombe, Rick P]

On Tue, 2022-05-24 at 10:22 +0300, Mike Rapoport wrote:
> > @@ -949,6 +947,8 @@ static void bpf_prog_pack_free(struct
> > bpf_binary_header *hdr)
> >   
> >        mutex_lock(&pack_mutex);
> >        if (hdr->size > bpf_prog_pack_size) {
> > +             set_memory_nx((unsigned long)hdr, hdr->size /
> > PAGE_SIZE);
> > +             set_memory_rw((unsigned long)hdr, hdr->size /
> > PAGE_SIZE);
> 
> set_memory_{nx,rw} can fail. Please take care of error handling.

I think there is nothing to do here, actually. At least on the freeing
part.

When called on a vmalloc primary address, the set_memory() calls will
try to first change the permissions on the vmalloc alias, then try to
change it on the direct map. Yea, it can fail from failing to allocate
a page from a split, but whatever memory managed to change earlier will
also succeed to change back on reset. The set_memory() functions don't
rollback on failure. So all the modules callers depend on this logic of
a second resetting call to reset anything that succeeded to change on
the first call. The split will have already happened for any memory
that succeeded to change, and the order of the changes is the same.

As for the primary alias, for 4k vmallocs, it will always succeed, and
set_memory() does this part first, so set_memory_x() will
(crucially) always succeed for kernel text mappings. For 2MB vmalloc
pages, the primary alias should succeed if the set_memory() call is 2MB
aligned.

So pre-VM_FLUSH_RESET_PERMS (and it also has pretty similar logic),
they all went something like this:
Allocate:
1. ptr = vmalloc();
2. set_memory_ro(ptr); <-Could fail halfway though

Free:
3. set_memory_rw(ptr); <-Could also fail halfway though and return an 
                         error, but only after the split work done in 
                         step 2.
4. vfree(ptr);

It's pretty horrible. BPF people once tried to be more proper and
change it to:
ptr = vmalloc()
if (set_memory_ro())
{
     vfree(ptr);
}

It looks correct, but this caused RO pages to be freed if
set_memory_ro() half succeeded in changing permissions. If there is an
error on reset, it means the first set_memory() call failed to change
everything, but anything that succeeded is reset. So the right thing to
do is to free the pages in either case.

We could fail to allocate a pack if the initial set_memory() calls
failed, but we still have to call set_memory_rw(), etc on free and
ignore any errors.

As an aside, this is one of the reasons it's hard to improve things
incrementally here. Everything is carefully balanced like a house of
cards. The solution IMO, is to change the interface so this type of
logic is in one place instead of scattered in the callers.

Re: [PATCH v4 bpf-next 5/8] bpf: use module_alloc_huge for bpf_prog_pack

[Edgecombe, Rick P]

On Fri, 2022-05-20 at 16:57 -0700, Song Liu wrote:
> Use module_alloc_huge for bpf_prog_pack so that BPF programs sit on
> PMD_SIZE pages. This benefits system performance by reducing iTLB
> miss
> rate. Benchmark of a real web service workload shows this change
> gives
> another ~0.2% performance boost on top of PAGE_SIZE bpf_prog_pack
> (which improve system throughput by ~0.5%).
> 
> Also, remove set_vm_flush_reset_perms() from alloc_new_pack() and use
> set_memory_[nx|rw] in bpf_prog_pack_free(). This is because
> VM_FLUSH_RESET_PERMS does not work with huge pages yet. [1]
> 
> [1] 
> https://lore.kernel.org/bpf/aeeeaf0b7ec63fdba55d4834d2f524d8bf05b71b.camel@intel.com/
> Suggested-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
> Signed-off-by: Song Liu <song@kernel.org>

Hi,

Per request, just wanted to make it clear I'm personally on board with
side-stepping VM_FLUSH_RESET_PERMS here because the way things are
headed it may change anyway, and this already does a better job at
minimizing eBPF JIT kernel shootdowns than VM_FLUSH_RESET_PERMS did
when it was introduced. The warning added in the other patch will
prevent accidents like the first version of the series. So that aspect
seems ok to me.

In general I think this series still needs more x86/mm scrutiny though.

Thanks,

Rick

Re: [PATCH v4 bpf-next 0/8] bpf_prog_pack followup

[Aaron Lu]

Hi Song,

On Fri, May 20, 2022 at 04:57:50PM -0700, Song Liu wrote:

... ...

> The primary goal of bpf_prog_pack is to reduce iTLB miss rate and reduce
> direct memory mapping fragmentation. This leads to non-trivial performance
> improvements.
>
> For our web service production benchmark, bpf_prog_pack on 4kB pages
> gives 0.5% to 0.7% more throughput than not using bpf_prog_pack.
> bpf_prog_pack on 2MB pages 0.6% to 0.9% more throughput than not using
> bpf_prog_pack. Note that 0.5% is a huge improvement for our fleet. I
> believe this is also significant for other companies with many thousand
> servers.
>

I'm evaluationg performance impact due to direct memory mapping
fragmentation and seeing the above, I wonder: is the performance improve
mostly due to prog pack and hugepage instead of less direct mapping
fragmentation?

I can understand that when progs are packed together, iTLB miss rate will
be reduced and thus, performance can be improved. But I don't see
immediately how direct mapping fragmentation can impact performance since
the bpf code are running from the module alias addresses, not the direct
mapping addresses IIUC?

I appreciate it if you can shed some light on performance impact direct
mapping fragmentation can cause, thanks.

Re: [PATCH v4 bpf-next 0/8] bpf_prog_pack followup

[Song Liu]

Hi Aaron,

On Mon, Jun 20, 2022 at 4:12 AM Aaron Lu <aaron.lu@intel.com> wrote:
>
> Hi Song,
>
> On Fri, May 20, 2022 at 04:57:50PM -0700, Song Liu wrote:
>
> ... ...
>
> > The primary goal of bpf_prog_pack is to reduce iTLB miss rate and reduce
> > direct memory mapping fragmentation. This leads to non-trivial performance
> > improvements.
> >
> > For our web service production benchmark, bpf_prog_pack on 4kB pages
> > gives 0.5% to 0.7% more throughput than not using bpf_prog_pack.
> > bpf_prog_pack on 2MB pages 0.6% to 0.9% more throughput than not using
> > bpf_prog_pack. Note that 0.5% is a huge improvement for our fleet. I
> > believe this is also significant for other companies with many thousand
> > servers.
> >
>
> I'm evaluationg performance impact due to direct memory mapping
> fragmentation and seeing the above, I wonder: is the performance improve
> mostly due to prog pack and hugepage instead of less direct mapping
> fragmentation?
>
> I can understand that when progs are packed together, iTLB miss rate will
> be reduced and thus, performance can be improved. But I don't see
> immediately how direct mapping fragmentation can impact performance since
> the bpf code are running from the module alias addresses, not the direct
> mapping addresses IIUC?

You are right that BPF code runs from module alias addresses. However, to
protect text from overwrites, we use set_memory_x() and set_memory_ro()
for the BPF code. These two functions will set permissions for all aliases
of the memory, including the direct map, and thus cause fragmentation of
the direct map. Does this make sense?

Thanks,
Song

Re: [PATCH v4 bpf-next 0/8] bpf_prog_pack followup

[Aaron Lu]

On Mon, Jun 20, 2022 at 09:03:52AM -0700, Song Liu wrote:
> Hi Aaron,
> 
> On Mon, Jun 20, 2022 at 4:12 AM Aaron Lu <aaron.lu@intel.com> wrote:
> >
> > Hi Song,
> >
> > On Fri, May 20, 2022 at 04:57:50PM -0700, Song Liu wrote:
> >
> > ... ...
> >
> > > The primary goal of bpf_prog_pack is to reduce iTLB miss rate and reduce
> > > direct memory mapping fragmentation. This leads to non-trivial performance
> > > improvements.
> > >
> > > For our web service production benchmark, bpf_prog_pack on 4kB pages
> > > gives 0.5% to 0.7% more throughput than not using bpf_prog_pack.
> > > bpf_prog_pack on 2MB pages 0.6% to 0.9% more throughput than not using
> > > bpf_prog_pack. Note that 0.5% is a huge improvement for our fleet. I
> > > believe this is also significant for other companies with many thousand
> > > servers.
> > >
> >
> > I'm evaluationg performance impact due to direct memory mapping
> > fragmentation and seeing the above, I wonder: is the performance improve
> > mostly due to prog pack and hugepage instead of less direct mapping
> > fragmentation?
> >
> > I can understand that when progs are packed together, iTLB miss rate will
> > be reduced and thus, performance can be improved. But I don't see
> > immediately how direct mapping fragmentation can impact performance since
> > the bpf code are running from the module alias addresses, not the direct
> > mapping addresses IIUC?
> 
> You are right that BPF code runs from module alias addresses. However, to
> protect text from overwrites, we use set_memory_x() and set_memory_ro()
> for the BPF code. These two functions will set permissions for all aliases
> of the memory, including the direct map, and thus cause fragmentation of
> the direct map. Does this make sense?

Guess I didn't make it clear.

I understand that set_memory_XXX() will cause direct mapping split and
thus, fragmented. What is not clear to me is, how much impact does
direct mapping fragmentation have on performance, in your case and in
general?

In your case, I guess the performance gain is due to code gets packed
together and iTLB gets reduced. When code are a lot, packing them
together as a hugepage is a further gain. In the meantime, direct
mapping split (or not) seems to be a side effect of this packing, but it
doesn't have a direct impact on performance.

One thing I can imagine is, when an area of direct mapping gets splited
due to permission reason, when that reason is gone(like module unload
or bpf code unload), those areas will remain fragmented and that can
cause later operations that touch these same areas using more dTLBs
and that can be bad for performance, but it's hard to say how much
impact this can cause though.

Regards,
Aaron

Re: [PATCH v4 bpf-next 0/8] bpf_prog_pack followup

[Song Liu]

On Mon, Jun 20, 2022 at 6:32 PM Aaron Lu <aaron.lu@intel.com> wrote:
>
> On Mon, Jun 20, 2022 at 09:03:52AM -0700, Song Liu wrote:
> > Hi Aaron,
> >
> > On Mon, Jun 20, 2022 at 4:12 AM Aaron Lu <aaron.lu@intel.com> wrote:
> > >
> > > Hi Song,
> > >
> > > On Fri, May 20, 2022 at 04:57:50PM -0700, Song Liu wrote:
> > >
> > > ... ...
> > >
> > > > The primary goal of bpf_prog_pack is to reduce iTLB miss rate and reduce
> > > > direct memory mapping fragmentation. This leads to non-trivial performance
> > > > improvements.
> > > >
> > > > For our web service production benchmark, bpf_prog_pack on 4kB pages
> > > > gives 0.5% to 0.7% more throughput than not using bpf_prog_pack.
> > > > bpf_prog_pack on 2MB pages 0.6% to 0.9% more throughput than not using
> > > > bpf_prog_pack. Note that 0.5% is a huge improvement for our fleet. I
> > > > believe this is also significant for other companies with many thousand
> > > > servers.
> > > >
> > >
> > > I'm evaluationg performance impact due to direct memory mapping
> > > fragmentation and seeing the above, I wonder: is the performance improve
> > > mostly due to prog pack and hugepage instead of less direct mapping
> > > fragmentation?
> > >
> > > I can understand that when progs are packed together, iTLB miss rate will
> > > be reduced and thus, performance can be improved. But I don't see
> > > immediately how direct mapping fragmentation can impact performance since
> > > the bpf code are running from the module alias addresses, not the direct
> > > mapping addresses IIUC?
> >
> > You are right that BPF code runs from module alias addresses. However, to
> > protect text from overwrites, we use set_memory_x() and set_memory_ro()
> > for the BPF code. These two functions will set permissions for all aliases
> > of the memory, including the direct map, and thus cause fragmentation of
> > the direct map. Does this make sense?
>
> Guess I didn't make it clear.
>
> I understand that set_memory_XXX() will cause direct mapping split and
> thus, fragmented. What is not clear to me is, how much impact does
> direct mapping fragmentation have on performance, in your case and in
> general?
>
> In your case, I guess the performance gain is due to code gets packed
> together and iTLB gets reduced. When code are a lot, packing them
> together as a hugepage is a further gain. In the meantime, direct
> mapping split (or not) seems to be a side effect of this packing, but it
> doesn't have a direct impact on performance.
>
> One thing I can imagine is, when an area of direct mapping gets splited
> due to permission reason, when that reason is gone(like module unload
> or bpf code unload), those areas will remain fragmented and that can
> cause later operations that touch these same areas using more dTLBs
> and that can be bad for performance, but it's hard to say how much
> impact this can cause though.

Yes, we have data showing the direct mapping remaining fragmented
can cause non-trivial performance degradation. For our web workload,
the difference is in the order of 1%.

Thanks,
Song

Re: [PATCH v4 bpf-next 0/8] bpf_prog_pack followup

[Aaron Lu]

On Mon, Jun 20, 2022 at 07:51:24PM -0700, Song Liu wrote:
> On Mon, Jun 20, 2022 at 6:32 PM Aaron Lu <aaron.lu@intel.com> wrote:
> >
> > On Mon, Jun 20, 2022 at 09:03:52AM -0700, Song Liu wrote:
> > > Hi Aaron,
> > >
> > > On Mon, Jun 20, 2022 at 4:12 AM Aaron Lu <aaron.lu@intel.com> wrote:
> > > >
> > > > Hi Song,
> > > >
> > > > On Fri, May 20, 2022 at 04:57:50PM -0700, Song Liu wrote:
> > > >
> > > > ... ...
> > > >
> > > > > The primary goal of bpf_prog_pack is to reduce iTLB miss rate and reduce
> > > > > direct memory mapping fragmentation. This leads to non-trivial performance
> > > > > improvements.
> > > > >
> > > > > For our web service production benchmark, bpf_prog_pack on 4kB pages
> > > > > gives 0.5% to 0.7% more throughput than not using bpf_prog_pack.
> > > > > bpf_prog_pack on 2MB pages 0.6% to 0.9% more throughput than not using
> > > > > bpf_prog_pack. Note that 0.5% is a huge improvement for our fleet. I
> > > > > believe this is also significant for other companies with many thousand
> > > > > servers.
> > > > >
> > > >
> > > > I'm evaluationg performance impact due to direct memory mapping
> > > > fragmentation and seeing the above, I wonder: is the performance improve
> > > > mostly due to prog pack and hugepage instead of less direct mapping
> > > > fragmentation?
> > > >
> > > > I can understand that when progs are packed together, iTLB miss rate will
> > > > be reduced and thus, performance can be improved. But I don't see
> > > > immediately how direct mapping fragmentation can impact performance since
> > > > the bpf code are running from the module alias addresses, not the direct
> > > > mapping addresses IIUC?
> > >
> > > You are right that BPF code runs from module alias addresses. However, to
> > > protect text from overwrites, we use set_memory_x() and set_memory_ro()
> > > for the BPF code. These two functions will set permissions for all aliases
> > > of the memory, including the direct map, and thus cause fragmentation of
> > > the direct map. Does this make sense?
> >
> > Guess I didn't make it clear.
> >
> > I understand that set_memory_XXX() will cause direct mapping split and
> > thus, fragmented. What is not clear to me is, how much impact does
> > direct mapping fragmentation have on performance, in your case and in
> > general?
> >
> > In your case, I guess the performance gain is due to code gets packed
> > together and iTLB gets reduced. When code are a lot, packing them
> > together as a hugepage is a further gain. In the meantime, direct
> > mapping split (or not) seems to be a side effect of this packing, but it
> > doesn't have a direct impact on performance.
> >
> > One thing I can imagine is, when an area of direct mapping gets splited
> > due to permission reason, when that reason is gone(like module unload
> > or bpf code unload), those areas will remain fragmented and that can
> > cause later operations that touch these same areas using more dTLBs
> > and that can be bad for performance, but it's hard to say how much
> > impact this can cause though.
> 
> Yes, we have data showing the direct mapping remaining fragmented
> can cause non-trivial performance degradation. For our web workload,
> the difference is in the order of 1%.

Many thanks for the info, really appreciate it.

Regards,
Aaron

Re: [PATCH v4 bpf-next 0/8] bpf_prog_pack followup

[Luis Chamberlain]

On Mon, Jun 20, 2022 at 07:11:45PM +0800, Aaron Lu wrote:
> Hi Song,
> 
> On Fri, May 20, 2022 at 04:57:50PM -0700, Song Liu wrote:
> 
> ... ...
> 
> > The primary goal of bpf_prog_pack is to reduce iTLB miss rate and reduce
> > direct memory mapping fragmentation. This leads to non-trivial performance
> > improvements.
> >
> > For our web service production benchmark, bpf_prog_pack on 4kB pages
> > gives 0.5% to 0.7% more throughput than not using bpf_prog_pack.
> > bpf_prog_pack on 2MB pages 0.6% to 0.9% more throughput than not using
> > bpf_prog_pack. Note that 0.5% is a huge improvement for our fleet. I
> > believe this is also significant for other companies with many thousand
> > servers.
> >
> 
> I'm evaluationg performance impact due to direct memory mapping
> fragmentation 

BTW how exactly are you doing this?

  Luis

> and seeing the above, I wonder: is the performance improve
> mostly due to prog pack and hugepage instead of less direct mapping
> fragmentation?
> 
> I can understand that when progs are packed together, iTLB miss rate will
> be reduced and thus, performance can be improved. But I don't see
> immediately how direct mapping fragmentation can impact performance since
> the bpf code are running from the module alias addresses, not the direct
> mapping addresses IIUC?
> 
> I appreciate it if you can shed some light on performance impact direct
> mapping fragmentation can cause, thanks.

Re: [PATCH v4 bpf-next 0/8] bpf_prog_pack followup

[Aaron Lu]

On Mon, Jun 20, 2022 at 11:31:39AM -0700, Luis Chamberlain wrote:
> On Mon, Jun 20, 2022 at 07:11:45PM +0800, Aaron Lu wrote:
> > Hi Song,
> > 
> > On Fri, May 20, 2022 at 04:57:50PM -0700, Song Liu wrote:
> > 
> > ... ...
> > 
> > > The primary goal of bpf_prog_pack is to reduce iTLB miss rate and reduce
> > > direct memory mapping fragmentation. This leads to non-trivial performance
> > > improvements.
> > >
> > > For our web service production benchmark, bpf_prog_pack on 4kB pages
> > > gives 0.5% to 0.7% more throughput than not using bpf_prog_pack.
> > > bpf_prog_pack on 2MB pages 0.6% to 0.9% more throughput than not using
> > > bpf_prog_pack. Note that 0.5% is a huge improvement for our fleet. I
> > > believe this is also significant for other companies with many thousand
> > > servers.
> > >
> > 
> > I'm evaluationg performance impact due to direct memory mapping
> > fragmentation 
> 
> BTW how exactly are you doing this?

Right now I'm mostly collecting materials from the web :-)

Zhengjun has run some extensive microbenmarks with different page size
for direct mapping and on different server machines a while ago, here
is his report:
https://lore.kernel.org/linux-mm/213b4567-46ce-f116-9cdf-bbd0c884eb3c@linux.intel.com/
Quoting part of the conclusion:
"
This leads us to conclude that although 1G mappings are a 
good default choice, there is no compelling evidence that it must be the 
only choice, or that folks deriving benefits (like hardening) from 
smaller mapping sizes should avoid the smaller mapping sizes.
"

I searched the archive and found there is performance problem when
kernel text huge mapping gets splitted:
https://lore.kernel.org/lkml/20190823052335.572133-1-songliubraving@fb.com/

But I haven't found a report complaining direct mapping fragmentation yet.