mbox series

[0/6] mm/vmalloc: Assorted fixes and improvements

Message ID 20230523135902.517032811@linutronix.de (mailing list archive)
Headers show
Series mm/vmalloc: Assorted fixes and improvements | expand

Message

Thomas Gleixner May 23, 2023, 2:02 p.m. UTC
Following up to the discussion about excessive TLB flushes

   https://lore.kernel.org/all/87a5y5a6kj.ffs@tglx

this series addresses the following issues:


  1) Prevent the stale TLB problem related to fully utilized vmap blocks


  2) Avoid the double per CPU list walk in _vm_unmap_aliases()


  3) Avoid flushing dirty space over and over


  4) Add a lockless quickcheck in vb_alloc() and add missing
     READ/WRITE_ONCE() annotations

  5) Prevent overeager purging of usable vmap_blocks if
     not under memory/address space pressure.

Thanks,

	tglx

Comments

Uladzislau Rezki May 23, 2023, 4:24 p.m. UTC | #1
On Tue, May 23, 2023 at 04:02:09PM +0200, Thomas Gleixner wrote:
> Following up to the discussion about excessive TLB flushes
> 
>    https://lore.kernel.org/all/87a5y5a6kj.ffs@tglx
> 
> this series addresses the following issues:
> 
> 
>   1) Prevent the stale TLB problem related to fully utilized vmap blocks
> 
> 
>   2) Avoid the double per CPU list walk in _vm_unmap_aliases()
> 
> 
>   3) Avoid flushing dirty space over and over
> 
> 
>   4) Add a lockless quickcheck in vb_alloc() and add missing
>      READ/WRITE_ONCE() annotations
> 
>   5) Prevent overeager purging of usable vmap_blocks if
>      not under memory/address space pressure.
> 
> Thanks,
> 
> 	tglx
Thank you for fixing it!

<snip>
urezki@pc638:~/data/raid0/coding/linux.git$ make -j64 bzImage
  DESCEND objtool
  INSTALL libsubcmd_headers
  CALL    scripts/checksyscalls.sh
  CC      mm/vmalloc.o
In file included from ./include/linux/list_lru.h:14,
                 from ./include/linux/fs.h:13,
                 from ./include/linux/huge_mm.h:8,
                 from ./include/linux/mm.h:855,
                 from mm/vmalloc.c:12:
mm/vmalloc.c: In function ‘_vm_unmap_aliases’:
mm/vmalloc.c:2220:19: error: ‘struct vmap_block_queue’ has no member named ‘vmap_blocks’
 2220 |   xa_for_each(&vbq->vmap_blocks, idx, vb) {
      |                   ^~
./include/linux/xarray.h:449:23: note: in definition of macro ‘xa_for_each_range’
  449 |       entry = xa_find(xa, &index, last, XA_PRESENT);  \
      |                       ^~
./include/linux/xarray.h:501:2: note: in expansion of macro ‘xa_for_each_start’
  501 |  xa_for_each_start(xa, index, entry, 0)
      |  ^~~~~~~~~~~~~~~~~
mm/vmalloc.c:2220:3: note: in expansion of macro ‘xa_for_each’
 2220 |   xa_for_each(&vbq->vmap_blocks, idx, vb) {
      |   ^~~~~~~~~~~
mm/vmalloc.c:2220:19: error: ‘struct vmap_block_queue’ has no member named ‘vmap_blocks’
 2220 |   xa_for_each(&vbq->vmap_blocks, idx, vb) {
      |                   ^~
./include/linux/xarray.h:451:29: note: in definition of macro ‘xa_for_each_range’
  451 |       entry = xa_find_after(xa, &index, last, XA_PRESENT))
      |                             ^~
./include/linux/xarray.h:501:2: note: in expansion of macro ‘xa_for_each_start’
  501 |  xa_for_each_start(xa, index, entry, 0)
      |  ^~~~~~~~~~~~~~~~~
mm/vmalloc.c:2220:3: note: in expansion of macro ‘xa_for_each’
 2220 |   xa_for_each(&vbq->vmap_blocks, idx, vb) {
      |   ^~~~~~~~~~~
mm/vmalloc.c:2228:9: error: too few arguments to function ‘purge_fragmented_block’
 2228 |    if (!purge_fragmented_block(vb, vbq, &purge_list) &&
      |         ^~~~~~~~~~~~~~~~~~~~~~
mm/vmalloc.c:2047:13: note: declared here
 2047 | static bool purge_fragmented_block(struct vmap_block *vb, struct vmap_block_queue *vbq,
      |             ^~~~~~~~~~~~~~~~~~~~~~
make[2]: *** [scripts/Makefile.build:252: mm/vmalloc.o] Error 1
make[1]: *** [scripts/Makefile.build:494: mm] Error 2
make[1]: *** Waiting for unfinished jobs....
make: *** [Makefile:2025: .] Error 2
urezki@pc638:~/data/raid0/coding/linux.git$ ^C
<snip>

Could please fix it? :)

--
Uladzislau Rezki
Thomas Gleixner May 23, 2023, 5:33 p.m. UTC | #2
On Tue, May 23 2023 at 18:24, Uladzislau Rezki wrote:
> On Tue, May 23, 2023 at 04:02:09PM +0200, Thomas Gleixner wrote:
> mm/vmalloc.c: In function ‘_vm_unmap_aliases’:
> mm/vmalloc.c:2220:19: error: ‘struct vmap_block_queue’ has no member named ‘vmap_blocks’
>  2220 |   xa_for_each(&vbq->vmap_blocks, idx, vb) {
>       |                   ^~

Duh. I surely had that compile fail fixed before I boot tested that
pile. And then I did something stupid obviously.
Thomas Gleixner May 23, 2023, 5:39 p.m. UTC | #3
On Tue, May 23 2023 at 19:33, Thomas Gleixner wrote:

> On Tue, May 23 2023 at 18:24, Uladzislau Rezki wrote:
>> On Tue, May 23, 2023 at 04:02:09PM +0200, Thomas Gleixner wrote:
>> mm/vmalloc.c: In function ‘_vm_unmap_aliases’:
>> mm/vmalloc.c:2220:19: error: ‘struct vmap_block_queue’ has no member named ‘vmap_blocks’
>>  2220 |   xa_for_each(&vbq->vmap_blocks, idx, vb) {
>>       |                   ^~
>
> Duh. I surely had that compile fail fixed before I boot tested that
> pile. And then I did something stupid obviously.

No. This one not. I only had the one in the last patch (missing
force_purge argument)

And this one makes me scratch my head:

struct vmap_block_queue {
	spinlock_t lock;
	struct list_head free;

	/*
	 * An xarray requires an extra memory dynamically to
	 * be allocated. If it is an issue, we can use rb-tree
	 * instead.
	 */
	struct xarray vmap_blocks;
};

So how can your compiler complain?

>> mm/vmalloc.c:2220:19: error: ‘struct vmap_block_queue’ has no member named ‘vmap_blocks’
>>  2220 |   xa_for_each(&vbq->vmap_blocks, idx, vb) {

Mine does not, but I might be missing something.

Thanks,

        tglx
Uladzislau Rezki May 23, 2023, 5:48 p.m. UTC | #4
On Tue, May 23, 2023 at 07:39:26PM +0200, Thomas Gleixner wrote:
> On Tue, May 23 2023 at 19:33, Thomas Gleixner wrote:
> 
> > On Tue, May 23 2023 at 18:24, Uladzislau Rezki wrote:
> >> On Tue, May 23, 2023 at 04:02:09PM +0200, Thomas Gleixner wrote:
> >> mm/vmalloc.c: In function ‘_vm_unmap_aliases’:
> >> mm/vmalloc.c:2220:19: error: ‘struct vmap_block_queue’ has no member named ‘vmap_blocks’
> >>  2220 |   xa_for_each(&vbq->vmap_blocks, idx, vb) {
> >>       |                   ^~
> >
> > Duh. I surely had that compile fail fixed before I boot tested that
> > pile. And then I did something stupid obviously.
> 
> No. This one not. I only had the one in the last patch (missing
> force_purge argument)
> 
> And this one makes me scratch my head:
> 
> struct vmap_block_queue {
> 	spinlock_t lock;
> 	struct list_head free;
> 
> 	/*
> 	 * An xarray requires an extra memory dynamically to
> 	 * be allocated. If it is an issue, we can use rb-tree
> 	 * instead.
> 	 */
> 	struct xarray vmap_blocks;
> };
>
> 
> So how can your compiler complain?
> 
> >> mm/vmalloc.c:2220:19: error: ‘struct vmap_block_queue’ has no member named ‘vmap_blocks’
> >>  2220 |   xa_for_each(&vbq->vmap_blocks, idx, vb) {
> 
> Mine does not, but I might be missing something.
> 
Right. I have applied your patches on the v6.3 what is not correct. I
thought it should be fine, because that part was not touched quite a
lot of time. Apparently, me, Lorenzo and Baoquan placed the vmap_blocks
under the vmap_block_queue structure.

So, v6.3 does not contain that patch. I have to use the next instead.

--
Uladzislau Rezki
Uladzislau Rezki May 23, 2023, 5:51 p.m. UTC | #5
On Tue, May 23, 2023 at 07:48:09PM +0200, Uladzislau Rezki wrote:
> On Tue, May 23, 2023 at 07:39:26PM +0200, Thomas Gleixner wrote:
> > On Tue, May 23 2023 at 19:33, Thomas Gleixner wrote:
> > 
> > > On Tue, May 23 2023 at 18:24, Uladzislau Rezki wrote:
> > >> On Tue, May 23, 2023 at 04:02:09PM +0200, Thomas Gleixner wrote:
> > >> mm/vmalloc.c: In function ‘_vm_unmap_aliases’:
> > >> mm/vmalloc.c:2220:19: error: ‘struct vmap_block_queue’ has no member named ‘vmap_blocks’
> > >>  2220 |   xa_for_each(&vbq->vmap_blocks, idx, vb) {
> > >>       |                   ^~
> > >
> > > Duh. I surely had that compile fail fixed before I boot tested that
> > > pile. And then I did something stupid obviously.
> > 
> > No. This one not. I only had the one in the last patch (missing
> > force_purge argument)
> > 
> > And this one makes me scratch my head:
> > 
> > struct vmap_block_queue {
> > 	spinlock_t lock;
> > 	struct list_head free;
> > 
> > 	/*
> > 	 * An xarray requires an extra memory dynamically to
> > 	 * be allocated. If it is an issue, we can use rb-tree
> > 	 * instead.
> > 	 */
> > 	struct xarray vmap_blocks;
> > };
> >
> > 
> > So how can your compiler complain?
> > 
> > >> mm/vmalloc.c:2220:19: error: ‘struct vmap_block_queue’ has no member named ‘vmap_blocks’
> > >>  2220 |   xa_for_each(&vbq->vmap_blocks, idx, vb) {
> > 
> > Mine does not, but I might be missing something.
> > 
> Right. I have applied your patches on the v6.3 what is not correct. I
> thought it should be fine, because that part was not touched quite a
> lot of time. Apparently, me, Lorenzo and Baoquan placed the vmap_blocks
> under the vmap_block_queue structure.
> 
> So, v6.3 does not contain that patch. I have to use the next instead.
> 
My fault :)

--
Uladzislau Rezki
Uladzislau Rezki May 23, 2023, 5:55 p.m. UTC | #6
On Tue, May 23, 2023 at 07:48:09PM +0200, Uladzislau Rezki wrote:
> On Tue, May 23, 2023 at 07:39:26PM +0200, Thomas Gleixner wrote:
> > On Tue, May 23 2023 at 19:33, Thomas Gleixner wrote:
> > 
> > > On Tue, May 23 2023 at 18:24, Uladzislau Rezki wrote:
> > >> On Tue, May 23, 2023 at 04:02:09PM +0200, Thomas Gleixner wrote:
> > >> mm/vmalloc.c: In function ‘_vm_unmap_aliases’:
> > >> mm/vmalloc.c:2220:19: error: ‘struct vmap_block_queue’ has no member named ‘vmap_blocks’
> > >>  2220 |   xa_for_each(&vbq->vmap_blocks, idx, vb) {
> > >>       |                   ^~
> > >
> > > Duh. I surely had that compile fail fixed before I boot tested that
> > > pile. And then I did something stupid obviously.
> > 
> > No. This one not. I only had the one in the last patch (missing
> > force_purge argument)
> > 
> > And this one makes me scratch my head:
> > 
> > struct vmap_block_queue {
> > 	spinlock_t lock;
> > 	struct list_head free;
> > 
> > 	/*
> > 	 * An xarray requires an extra memory dynamically to
> > 	 * be allocated. If it is an issue, we can use rb-tree
> > 	 * instead.
> > 	 */
> > 	struct xarray vmap_blocks;
> > };
> >
> > 
> > So how can your compiler complain?
> > 
> > >> mm/vmalloc.c:2220:19: error: ‘struct vmap_block_queue’ has no member named ‘vmap_blocks’
> > >>  2220 |   xa_for_each(&vbq->vmap_blocks, idx, vb) {
> > 
> > Mine does not, but I might be missing something.
> > 
> Right. I have applied your patches on the v6.3 what is not correct. I
> thought it should be fine, because that part was not touched quite a
> lot of time. Apparently, me, Lorenzo and Baoquan placed the vmap_blocks
> under the vmap_block_queue structure.
> 
> So, v6.3 does not contain that patch. I have to use the next instead.
> 
next-20230523:

mm/vmalloc.c: In function ‘_vm_unmap_aliases’:
mm/vmalloc.c:2280:9: error: too few arguments to function ‘purge_fragmented_block’
 2280 |    if (!purge_fragmented_block(vb, vbq, &purge_list) &&
      |         ^~~~~~~~~~~~~~~~~~~~~~
mm/vmalloc.c:2095:13: note: declared here
 2095 | static bool purge_fragmented_block(struct vmap_block *vb, struct vmap_block_queue *vbq,
      |             ^~~~~~~~~~~~~~~~~~~~~~
  CC      drivers/acpi/pmic/intel_pmic_bxtwc.o
 

there is only one complain in fact.

--
Uladzislau Rezki
Thomas Gleixner May 23, 2023, 6:40 p.m. UTC | #7
On Tue, May 23 2023 at 19:55, Uladzislau Rezki wrote:
> On Tue, May 23, 2023 at 07:48:09PM +0200, Uladzislau Rezki wrote:
>> So, v6.3 does not contain that patch. I have to use the next instead.
>> 
> next-20230523:

It works against linus tree just fine.

> mm/vmalloc.c: In function ‘_vm_unmap_aliases’:
> mm/vmalloc.c:2280:9: error: too few arguments to function ‘purge_fragmented_block’
>  2280 |    if (!purge_fragmented_block(vb, vbq, &purge_list) &&
>       |         ^~~~~~~~~~~~~~~~~~~~~~
> mm/vmalloc.c:2095:13: note: declared here
>  2095 | static bool purge_fragmented_block(struct vmap_block *vb, struct vmap_block_queue *vbq,
>       |             ^~~~~~~~~~~~~~~~~~~~~~
>   CC      drivers/acpi/pmic/intel_pmic_bxtwc.o
>  
>
> there is only one complain in fact.

That's the one where I failed to pull the refreshed patch back from the
test machine.

---
Subject: mm/vmalloc: Don't purge usable blocks unnecessarily
From: Thomas Gleixner <tglx@linutronix.de>
Date: Mon, 22 May 2023 19:38:32 +0200

Purging fragmented blocks is done unconditionally in several contexts:

  1) From drain_vmap_area_work(), when the number of lazy to be freed
     vmap_areas reached the threshold

  2) Reclaiming vmalloc address space from pcpu_get_vm_areas()

  3) _unmap_aliases()

#1 There is no reason to zap fragmented vmap blocks unconditionally, simply
   because reclaiming all lazy areas drains at least

      32MB * fls(num_online_cpus())

   per invocation which is plenty.

#2 Reclaiming when running out of space or due to memory pressure makes a
   lot of sense

#3 _unmap_aliases() requires to touch everything because the caller has no
   clue which vmap_area used a particular page last and the vmap_area lost
   that information too.

   Except for the vfree + VM_FLUSH_RESET_PERMS case, which removes the
   vmap area first and then cares about the flush. That in turn requires
   a full walk of _all_ vmap areas including the one which was just
   added to the purge list.

   But as this has to be flushed anyway this is an opportunity to combine
   outstanding TLB flushes and do the housekeeping of purging freed areas,
   but like #1 there is no real good reason to zap usable vmap blocks
   unconditionally.

Add a @force_purge argument to the relevant functions and if not true only
purge fragmented blocks which have less than 1/4 of their capacity left.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 mm/vmalloc.c |   36 +++++++++++++++++++++++-------------
 1 file changed, 23 insertions(+), 13 deletions(-)

--- a/mm/vmalloc.c
+++ b/mm/vmalloc.c
@@ -791,7 +791,7 @@ get_subtree_max_size(struct rb_node *nod
 RB_DECLARE_CALLBACKS_MAX(static, free_vmap_area_rb_augment_cb,
 	struct vmap_area, rb_node, unsigned long, subtree_max_size, va_size)
 
-static void purge_vmap_area_lazy(void);
+static void purge_vmap_area_lazy(bool force_purge);
 static BLOCKING_NOTIFIER_HEAD(vmap_notify_list);
 static void drain_vmap_area_work(struct work_struct *work);
 static DECLARE_WORK(drain_vmap_work, drain_vmap_area_work);
@@ -1649,7 +1649,7 @@ static struct vmap_area *alloc_vmap_area
 
 overflow:
 	if (!purged) {
-		purge_vmap_area_lazy();
+		purge_vmap_area_lazy(true);
 		purged = 1;
 		goto retry;
 	}
@@ -1717,7 +1717,7 @@ static atomic_long_t vmap_lazy_nr = ATOM
 static DEFINE_MUTEX(vmap_purge_lock);
 
 /* for per-CPU blocks */
-static void purge_fragmented_blocks_allcpus(void);
+static void purge_fragmented_blocks_allcpus(bool force_purge);
 
 /*
  * Purges all lazily-freed vmap areas.
@@ -1787,10 +1787,10 @@ static bool __purge_vmap_area_lazy(unsig
 /*
  * Kick off a purge of the outstanding lazy areas.
  */
-static void purge_vmap_area_lazy(void)
+static void purge_vmap_area_lazy(bool force_purge)
 {
 	mutex_lock(&vmap_purge_lock);
-	purge_fragmented_blocks_allcpus();
+	purge_fragmented_blocks_allcpus(force_purge);
 	__purge_vmap_area_lazy(ULONG_MAX, 0);
 	mutex_unlock(&vmap_purge_lock);
 }
@@ -1908,6 +1908,12 @@ static struct vmap_area *find_unlink_vma
 
 #define VMAP_BLOCK_SIZE		(VMAP_BBMAP_BITS * PAGE_SIZE)
 
+/*
+ * Purge threshold to prevent overeager purging of fragmented blocks for
+ * regular operations: Purge if vb->free is less than 1/4 of the capacity.
+ */
+#define VMAP_PURGE_THRESHOLD	(VMAP_BBMAP_BITS / 4)
+
 #define VMAP_RAM		0x1 /* indicates vm_map_ram area*/
 #define VMAP_BLOCK		0x2 /* mark out the vmap_block sub-type*/
 #define VMAP_FLAGS_MASK		0x3
@@ -2087,12 +2093,16 @@ static void free_vmap_block(struct vmap_
 }
 
 static bool purge_fragmented_block(struct vmap_block *vb, struct vmap_block_queue *vbq,
-				   struct list_head *purge_list)
+				   struct list_head *purge_list, bool force_purge)
 {
 	if (!(vb->free + vb->dirty == VMAP_BBMAP_BITS && vb->dirty != VMAP_BBMAP_BITS))
 		return false;
 
-	 /* prevent further allocs after releasing lock */
+	/* Don't overeagerly purge usable blocks unless requested */
+	if (!force_purge && vb->free < VMAP_PURGE_THRESHOLD)
+		return false;
+
+	/* prevent further allocs after releasing lock */
 	WRITE_ONCE(vb->free, 0);
 	 /* prevent purging it again */
 	WRITE_ONCE(vb->dirty, VMAP_BBMAP_BITS);
@@ -2115,7 +2125,7 @@ static void free_purged_blocks(struct li
 	}
 }
 
-static void purge_fragmented_blocks(int cpu)
+static void purge_fragmented_blocks(int cpu, bool force_purge)
 {
 	LIST_HEAD(purge);
 	struct vmap_block *vb;
@@ -2130,19 +2140,19 @@ static void purge_fragmented_blocks(int
 			continue;
 
 		spin_lock(&vb->lock);
-		purge_fragmented_block(vb, vbq, &purge);
+		purge_fragmented_block(vb, vbq, &purge, force_purge);
 		spin_unlock(&vb->lock);
 	}
 	rcu_read_unlock();
 	free_purged_blocks(&purge);
 }
 
-static void purge_fragmented_blocks_allcpus(void)
+static void purge_fragmented_blocks_allcpus(bool force_purge)
 {
 	int cpu;
 
 	for_each_possible_cpu(cpu)
-		purge_fragmented_blocks(cpu);
+		purge_fragmented_blocks(cpu, force_purge);
 }
 
 static void *vb_alloc(unsigned long size, gfp_t gfp_mask)
@@ -2267,7 +2277,7 @@ static void _vm_unmap_aliases(unsigned l
 			 * not purgeable, check whether there is dirty
 			 * space to be flushed.
 			 */
-			if (!purge_fragmented_block(vb, vbq, &purge_list) &&
+			if (!purge_fragmented_block(vb, vbq, &purge_list, false) &&
 			    vb->dirty_max && vb->dirty != VMAP_BBMAP_BITS) {
 				unsigned long va_start = vb->va->va_start;
 				unsigned long s, e;
@@ -4173,7 +4183,7 @@ struct vm_struct **pcpu_get_vm_areas(con
 overflow:
 	spin_unlock(&free_vmap_area_lock);
 	if (!purged) {
-		purge_vmap_area_lazy();
+		purge_vmap_area_lazy(true);
 		purged = true;
 
 		/* Before "retry", check if we recover. */