Date:   Thu, 2 Feb 2023 21:18:23 +0800
From:   Baoquan He <bhe@redhat.com>
To:     Lorenzo Stoakes <lstoakes@gmail.com>
Cc:     linux-kernel@vger.kernel.org, linux-mm@kvack.org,
        akpm@linux-foundation.org, stephen.s.brennan@oracle.com,
        urezki@gmail.com, willy@infradead.org, hch@infradead.org,
        error27@gmail.com
Subject: Re: [PATCH v4 3/7] mm/vmalloc.c: allow vread() to read out
 vm_map_ram areas
Message-ID: <Y9u3+vQqE5e/EXeO@MiWiFi-R3L-srv>
References: <20230201091339.61761-1-bhe@redhat.com>
 <20230201091339.61761-4-bhe@redhat.com>
 <Y9rImPGSF/EQ1Xey@lucifer>
 <Y9sr56DqC+JCXt7z@MiWiFi-R3L-srv>
 <Y9tjnP2r7RqbFd7a@lucifer>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <Y9tjnP2r7RqbFd7a@lucifer>
Precedence: bulk

On 02/02/23 at 07:17am, Lorenzo Stoakes wrote:
> On Thu, Feb 02, 2023 at 11:20:07AM +0800, Baoquan He wrote:
> 
> [snip]
> 
> > > > +	for_each_set_bitrange(rs, re, vb->used_map, VMAP_BBMAP_BITS) {
> > > > +		if (!count)
> > > > +			break;
> > > > +		start = vmap_block_vaddr(vb->va->va_start, rs);
> > > > +		while (addr < start) {
> > > > +			if (count == 0)
> > > > +				break;
> > >
> > > Bit pedantic, but you're using the `if (!count)` form of checking whether it's
> > > zero above, but here you explicitly check it, would be good to keep both consistent.
> >
> > Yeah, sounds good. Will change.
> >
> > >
> > > Given you're checking here, perhaps you could simply drop the previous check?
> >
> > Well, maybe no. The previous "if (!count)" is checking if count is 0
> > after the 'count -=n;' line at the end of the for_each loop. While this
> > "if (count == 0)" is checking if count is 0 after 'count--;' at the end
> > of while loop. Not sure if I got your point.
> 
> You're right, sorry each break is for a different loop :) and I guess the inner
> check is feeding the outer one so we're all good.

Oh, the inner check and break only terminates the while loop, but it
should jump to the 'spin_unlock(&vb->lock);' line too as the outer
break does. I will fix this.