On Wed, May 17 2023 at 18:52, Baoquan He wrote:
> On 05/17/23 at 11:38am, Thomas Gleixner wrote:
>> On Tue, May 16 2023 at 21:03, Thomas Gleixner wrote:
>> >
>> > Aside of that, if I read the code correctly then if there is an unmap
>> > via vb_free() which does not cover the whole vmap block then vb->dirty
>> > is set and every _vm_unmap_aliases() invocation flushes that dirty range
>> > over and over until that vmap block is completely freed, no?
>>
>> Something like the below would cure that.
>>
>> While it prevents that this is flushed forever it does not cure the
>> eventually overly broad flush when the block is completely dirty and
>> purged:
>>
>> Assume a block with 1024 pages, where 1022 pages are already freed and
>> TLB flushed. Now the last 2 pages are freed and the block is purged,
>> which results in a flush of 1024 pages where 1022 are already done,
>> right?
>
> This is good idea, I am thinking how to reply to your last mail and how
> to fix this. While your cure code may not work well. Please see below
> inline comment.
See below.
> One vmap block has 64 pages.
> #define VMAP_MAX_ALLOC BITS_PER_LONG /* 256K with 4K pages */
No, VMAP_MAX_ALLOC is the allocation limit for a single vb_alloc().
On 64bit it has at least 128 pages, but can have up to 1024:
#define VMAP_BBMAP_BITS_MAX 1024 /* 4MB with 4K pages */
#define VMAP_BBMAP_BITS_MIN (VMAP_MAX_ALLOC*2)
and then some magic happens to calculate the actual size
#define VMAP_BBMAP_BITS \
VMAP_MIN(VMAP_BBMAP_BITS_MAX, \
VMAP_MAX(VMAP_BBMAP_BITS_MIN, \
VMALLOC_PAGES / roundup_pow_of_two(NR_CPUS) / 16))
which is in a range of (2*BITS_PER_LONG) ... 1024.
The actual vmap block size is:
#define VMAP_BLOCK_SIZE (VMAP_BBMAP_BITS * PAGE_SIZE)
Which is then obviously something between 512k and 4MB on 64bit and
between 256k and 4MB on 32bit.
>> @@ -2240,13 +2240,17 @@ static void _vm_unmap_aliases(unsigned l
>> rcu_read_lock();
>> list_for_each_entry_rcu(vb, &vbq->free, free_list) {
>> spin_lock(&vb->lock);
>> - if (vb->dirty && vb->dirty != VMAP_BBMAP_BITS) {
>> + if (vb->dirty_max && vb->dirty != VMAP_BBMAP_BITS) {
>> unsigned long va_start = vb->va->va_start;
>> unsigned long s, e;
>
> When vb_free() is invoked, it could cause three kinds of vmap_block as
> below. Your code works well for the 2nd case, for the 1st one, it may be
> not. And the 2nd one is the stuff that we reclaim and put into purge
> list in purge_fragmented_blocks_allcpus().
>
> 1)
> |-----|------------|-----------|-------|
> |dirty|still mapped| dirty | free |
>
> 2)
> |------------------------------|-------|
> | dirty | free |
You sure? The first one is put into the purge list too.
/* Expand dirty range */
vb->dirty_min = min(vb->dirty_min, offset);
vb->dirty_max = max(vb->dirty_max, offset + (1UL << order));
pages bits dirtymin dirtymax
vb_alloc(A) 2 0 - 1 VMAP_BBMAP_BITS 0
vb_alloc(B) 4 2 - 5
vb_alloc(C) 2 6 - 7
So you get three variants:
1) Flush after freeing A
vb_free(A) 2 0 - 1 0 1
Flush VMAP_BBMAP_BITS 0 <- correct
vb_free(C) 2 6 - 7 6 7
Flush VMAP_BBMAP_BITS 0 <- correct
2) No flush between freeing A and C
vb_free(A) 2 0 - 1 0 1
vb_free(C) 2 6 - 7 0 7
Flush VMAP_BBMAP_BITS 0 <- overbroad flush
3) No flush between freeing A, C, B
vb_free(A) 2 0 - 1 0 1
vb_free(C) 2 6 - 7 0 7
vb_free(C) 2 2 - 5 0 7
Flush VMAP_BBMAP_BITS 0 <- correct
So my quick hack makes it correct for #1 and #3 and prevents repeated
flushes of already flushed areas.
To prevent #2 you need a bitmap which keeps track of the flushed areas.
Thanks,
tglx
