On Wed, May 20, 2020 at 11:44:57AM +0200, Jan Klemkow wrote:
> The function km_alloc() returns the uninitialized local variable sva if
> pgl is empty. It seems to be not possible in the current condition of
> the code, but I'm not sure if this is guaranteed. Thus, I would prefer
> to initialize sva with zero.
I think initializing sva to 0 is better than returning some stack
memory if something goes wrong.
> It also seems to be unnecessary to loop over the whole pagelist to find
> the first element. The marco pmap_map_direct() just does some
> calculations and the value of va is discarded.
It is not a macro on other architectures. You must make sure that
it has no side effects everywhere. I would not touch this.
> I build and run the code on amd64 without any issue and regress/sys/uvm
> also doesn't show any problems with that diff.
Only testing amd64 is not enough for such a diff.
bluhm
> Index: uvm/uvm_km.c
> ===================================================================
> RCS file: /cvs/src/sys/uvm/uvm_km.c,v
> retrieving revision 1.136
> diff -u -p -r1.136 uvm_km.c
> --- uvm/uvm_km.c 18 Feb 2020 12:13:40 -0000 1.136
> +++ uvm/uvm_km.c 20 May 2020 06:17:41 -0000
> @@ -816,7 +816,7 @@ km_alloc(size_t sz, const struct kmem_va
> paddr_t pla_align;
> int pla_flags;
> int pla_maxseg;
> - vaddr_t va, sva;
> + vaddr_t va, sva = 0;
>
> KASSERT(sz == round_page(sz));
>
> @@ -851,11 +851,8 @@ km_alloc(size_t sz, const struct kmem_va
> * allocations.
> */
> if (kv->kv_singlepage || kp->kp_maxseg == 1) {
> - TAILQ_FOREACH(pg, &pgl, pageq) {
> - va = pmap_map_direct(pg);
> - if (pg == TAILQ_FIRST(&pgl))
> - sva = va;
> - }
> + if ((pg = TAILQ_FIRST(&pgl)) != NULL)
> + sva = pmap_map_direct(pg);
> return ((void *)sva);
> }
> #endif
