Page MenuHomeFreeBSD

vm_phys_init: Quiet unused but set warnings about npages.
ClosedPublic

Authored by jhb on Apr 12 2022, 6:31 PM.

Details

Summary

npages is used in two optional cases:

  • to conditionally create a separate DMA32 free list
  • to index vm_page_array for VM_PHYSSEG_SPARSE

Add in more #ifdef's around npages statements.

Diff Detail

Repository
rG FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

jhb requested review of this revision.Apr 12 2022, 6:31 PM

I really dislike all the ifdefs and would be tempted to define VM_DMA32_NPAGES_THRESHOLD to 0 if it's not defined. (And currently it's defined iff VM_FREELIST_DMA32 is defined anyway.) But this is ok.

This revision is now accepted and ready to land.Apr 12 2022, 6:45 PM

I really dislike all the ifdefs and would be tempted to define VM_DMA32_NPAGES_THRESHOLD to 0 if it's not defined. (And currently it's defined iff VM_FREELIST_DMA32 is defined anyway.) But this is ok.

I would be fine with defaulting VM_DMA32_NPAGES_THRESHOLD to 0. I think npages declaration is still stuck with being under #ifdef's though as the read is only under #ifdef VM_FREELIST_DMA32 so you'd need all the new #ifdef's just with that name instead. (Right now I'm assuming that VM_DMA32_NPAGES_THRESHOLD basically implies VM_FREELIST_DMA32.)

sys/vm/vm_phys.c
515

The code here also seems wrong. The first segment below VM_DMA32_BOUNDARY will see a npages of 0 and will fall through to the else case and count those pages as "> 4G" pages even though they aren't. I feel like the logic you kind of want is more like:

bool seen_dma32;
...
seen_dma32 = false;
...
#ifdef VM_FREELIST_DMA32
       if (seg->end <= VM_DMA32_BOUNDARY) {
           seen_dma32 = true;
#ifdef VM_DMA32_NPAGES_THRESHOLD
           if n(pages > VM_DMA32_NPAGES_THRESHOLD)
#endif
               vm_freelist_to_flind[VM_FREELIST_DMA32] = 1;
       else {
#ifdef VM_DMA32_NPAGES_THRESHOLD
           npages += atop(seg->end - seg->start);
#endif
#else
       {
#endif
           vm_freelist_to_flind[VM_FREELIST_DEFAULT] = 1;
       }

Default VM_DMA32_NPAGES_THRESHOLD to 0 would then just leave the VM_FREELIST_DMA32 #ifdef which is slightly less terrible?

jhb retitled this revision from vm_phys_init: Quite unused but set warnings about npages. to vm_phys_init: Quiet unused but set warnings about npages..Apr 12 2022, 9:03 PM
In D34887#790746, @jhb wrote:

I really dislike all the ifdefs and would be tempted to define VM_DMA32_NPAGES_THRESHOLD to 0 if it's not defined. (And currently it's defined iff VM_FREELIST_DMA32 is defined anyway.) But this is ok.

I would be fine with defaulting VM_DMA32_NPAGES_THRESHOLD to 0. I think npages declaration is still stuck with being under #ifdef's though as the read is only under #ifdef VM_FREELIST_DMA32 so you'd need all the new #ifdef's just with that name instead. (Right now I'm assuming that VM_DMA32_NPAGES_THRESHOLD basically implies VM_FREELIST_DMA32.)

Yeah, adding a default value for this threshold would just let us remove some of the existing ifdefs.

sys/vm/vm_phys.c
515

Note that the loop steps over the segs in reverse order, i.e., starting from the highest PAs. I agree that the comment still isn't quite correct, though. We are indeed counting pages below 4G towards the threshold. I don't think it matters very much and would be inclined to fix the comment instead.

jhb marked an inline comment as done.Apr 13 2022, 10:38 PM
jhb added inline comments.
sys/vm/vm_phys.c
515

Ok. I'd be happy to have you fix the comment. :)

This revision was automatically updated to reflect the committed changes.
jhb marked an inline comment as done.