CFR: extend use of nitems() macro in the kernel.
John Baldwin
jhb at freebsd.org
Mon Apr 18 18:53:48 UTC 2016
On Saturday, April 16, 2016 01:25:09 PM Pedro Giffuni wrote:
> Hello;
>
> Using coccinelle, and some hand re-formatting, I generated a patch to
> make use of the nitems() macro in sys, which is too big for
> phabricator [1].
>
> I was careful to exclude anything from the contrib directory or
> any code that is shared with userland (as to not have to include
> additional headers).
>
> The patch is big but pretty safe, I think. The changes are small but
> still it touches many files[1].
>
> I would like some feedback on the convenience of doing such replacement.
> If it is a good idea we could do the same for roundup2() and, in fact,
> I think DragonFly has already done this.
>
> Regards,
>
> Pedro.
>
> [1] https://people.freebsd.org/~pfg/patches/sys-nitems.diff
>
> [2] For those too lazy to check [1], here is a list of affected files.
>
> M sys/amd64/amd64/amd64_mem.c
> M sys/amd64/amd64/machdep.c
> M sys/amd64/linux/linux_sysvec.c
> M sys/amd64/linux32/linux32_sysvec.c
> M sys/arm/amlogic/aml8726/aml8726_clkmsr.c
> M sys/arm/arm/db_interface.c
> M sys/arm/at91/at91_pmc.c
> M sys/arm/mv/armadaxp/armadaxp.c
> M sys/arm/ti/cpsw/if_cpsw.c
> M sys/arm/xscale/ixp425/ixp425.c
> M sys/boot/common/part.c
> M sys/boot/efi/loader/bootinfo.c
> M sys/boot/mips/beri/boot2/boot2.c
> M sys/boot/uboot/common/metadata.c
> M sys/cam/ata/ata_da.c
> M sys/cam/ata/ata_xpt.c
I would perhaps remove ata_quirk_table_size entirely and replace its
use with nitems() directly. Probably best if that was a separate commit
though from the near-mechanical replacement.
> M sys/cam/cam.c
Same here. num_cam_status_entries is only used in one place and I think
directly using nitems() is probably clearer overall.
> M sys/cam/scsi/scsi_all.c
Possibly the same here with sense_key_table_size.
> M sys/cam/scsi/scsi_cd.c
> M sys/cam/scsi/scsi_da.c
> M sys/cam/scsi/scsi_sa.c
> M sys/cam/scsi/scsi_xpt.c
Same as for ata_quirk_table_size (ironically this used the size in one
place and the expanded form of nitems in another while the ata variant
at least used the size in both places).
> M sys/cam/scsi/smp_all.c
> M sys/compat/linux/linux_socket.c
> M sys/ddb/db_variables.c
> M sys/dev/adb/adb_kbd.c
> M sys/dev/advansys/adv_isa.c
> M sys/dev/advansys/advlib.c
> M sys/dev/advansys/adw_pci.c
Same here for num adw_num_pci_devs (only used once)
> M sys/dev/advansys/adwlib.c
Probably the same here for adw_num_sync_rates (used twice).
> M sys/dev/ae/if_ae.c
Same here for AE_DEVS_COUNT (only used once).
> M sys/dev/age/if_age.c
> M sys/dev/agp/agp.c
> M sys/dev/agp/agp_ali.c
> M sys/dev/agp/agp_amd64.c
> M sys/dev/aha/aha_isa.c
> M sys/dev/aic/aic.c
> M sys/dev/aic/aic_cbus.c
Same here for AIC_ISA_NUMPORTS (only used once).
> M sys/dev/aic/aic_isa.c
As above.
> M sys/dev/ale/if_ale.c
> M sys/dev/altera/atse/if_atse.c
> M sys/dev/atkbdc/atkbd.c
> M sys/dev/atkbdc/atkbdc.c
> M sys/dev/atkbdc/psm.c
> M sys/dev/bktr/bktr_core.c
> M sys/dev/bwi/bwirf.c
> M sys/dev/bwn/if_bwn.c
> M sys/dev/cardbus/cardbus_cis.c
> M sys/dev/digi/digi.c
> M sys/dev/digi/digi_isa.c
> M sys/dev/dwc/if_dwc.c
> M sys/dev/ed/if_ed_hpp.c
> M sys/dev/ed/if_ed_isa.c
> M sys/dev/ed/if_ed_pci.c
> M sys/dev/fb/creator.c
Same here for CREATOR_FB_MAP_SIZE (only used once).
> M sys/dev/fb/fb.c
> M sys/dev/fb/machfb.c
> M sys/dev/fb/vesa.c
> M sys/dev/fb/vga.c
> M sys/dev/flash/mx25l.c
> M sys/dev/hatm/if_hatm.c
> M sys/dev/hifn/hifn7751.c
> M sys/dev/hwpmc/hwpmc_amd.c
Same here for amd_event_codes_size (only used twice).
> M sys/dev/hwpmc/hwpmc_core.c
Same here for niap_events.
> M sys/dev/hwpmc/hwpmc_e500.c
e500_event_codes_size isn't even used. It should just be removed.
> M sys/dev/hwpmc/hwpmc_mips24k.c
> M sys/dev/hwpmc/hwpmc_mips74k.c
> M sys/dev/hwpmc/hwpmc_mpc7xxx.c
Same here for mpc7xxx_event_codes_size.
> M sys/dev/hwpmc/hwpmc_octeon.c
> M sys/dev/hwpmc/hwpmc_uncore.c
Same here for nucp_events.
> M sys/dev/hwpmc/hwpmc_xscale.c
Same here for xscale_event_codes_size.
> M sys/dev/if_ndis/if_ndis.c
> M sys/dev/jme/if_jme.c
> M sys/dev/kbd/kbd.c
> M sys/dev/le/if_le_isa.c
> M sys/dev/le/if_le_lebuffer.c
Same here for NLEMEDIA (only used once).
> M sys/dev/le/if_le_ledma.c
> M sys/dev/mlx/mlx.c
> M sys/dev/mxge/if_mxge.c
> M sys/dev/nand/nand_id.c
> M sys/dev/ncr/ncr.c
> M sys/dev/nctgpio/nctgpio.c
> M sys/dev/nfe/if_nfe.c
> M sys/dev/patm/if_patm_attach.c
> M sys/dev/pccard/pccard_cis_quirks.c
Same here for n_pccard_cis_quirks (only used once).
> M sys/dev/rc/rc.c
> M sys/dev/re/if_re.c
> M sys/dev/rl/if_rl.c
> M sys/dev/rndtest/rndtest.c
Same here for RNDTEST_NTESTS (only used once).
> M sys/dev/sf/if_sf.c
> M sys/dev/sge/if_sge.c
Same here for 'cnt' (only used once).
> M sys/dev/siba/siba.c
Same here for 'bound'. I would actually simplify this function
even further so it just does 'return (sd)' instead of 'break'
in the loop body. This means you can remove the '==' check
and just 'return (NULL)' if the loop completes. It also means
that 'bound' is then only used once.
> M sys/dev/sio/sio.c
> M sys/dev/sound/isa/gusc.c
> M sys/dev/sound/pci/emu10kx.c
Same here for 'n_cards' (it is only used once after each assignment).
> M sys/dev/speaker/spkr.c
> M sys/dev/stge/if_stge.c
> M sys/dev/uart/uart_kbd_sun.c
> M sys/dev/uart/uart_subr.c
Same here for 'uart_nclasses' (only used once).
> M sys/dev/usb/input/ukbd.c
> M sys/dev/usb/serial/u3g.c
> M sys/dev/usb/serial/uchcom.c
Same here for NUM_DIVIDERS (only used once).
> M sys/dev/usb/serial/umcs.c
> M sys/dev/usb/serial/uplcom.c
Same here for N_UPLCOM_RATES (only used once).
> M sys/dev/vkbd/vkbd.c
> M sys/dev/wbwd/wbwd.c
> M sys/fs/autofs/autofs.c
> M sys/fs/nfs/nfs_commonkrpc.c
> M sys/geom/part/g_part_bsd.c
> M sys/geom/part/g_part_ebr.c
> M sys/geom/part/g_part_ldm.c
> M sys/geom/part/g_part_mbr.c
> M sys/i386/i386/i686_mem.c
> M sys/i386/i386/machdep.c
> M sys/i386/ibcs2/ibcs2_sysvec.c
> M sys/i386/linux/linux_sysvec.c
> M sys/kern/kern_dump.c
> M sys/kern/kern_ffclock.c
> M sys/kern/kern_jail.c
> M sys/kern/kern_ktrace.c
> M sys/kern/subr_hash.c
Same here for NPRIMES (only used once).
> M sys/kern/subr_witness.c
Same here for blessed_count (only used once).
> M sys/kern/sysv_msg.c
> M sys/kern/sysv_sem.c
> M sys/kern/uipc_usrreq.c
> M sys/mips/mips/db_interface.c
> M sys/mips/nlm/board.c
> M sys/mips/nlm/xlp_machdep.c
Same here for nreg (only used once).
> M sys/mips/rmi/dev/nlge/if_nlge.c
Same here for n_gmac_buckers and n_regs (each only used once).
> M sys/net/netisr.c
I would do the same here for netisr_dispatch_table_len. It is
used in three loops, but in all three the code would be:
for (i = 0; i < nitems(foo); i++) {
/* use foo[i] */
}
For that idiom, I think using nitems is clearer to the reader vs one of
NFOO, nfoo, foo_size, foo_len, etc. if for no other reason than
consistency. I think it is also more explicit.
> M sys/net/rtsock.c
> M sys/netgraph/atm/ng_atm.c
> M sys/netgraph/bluetooth/socket/ng_btsocket.c
Same here for ng_btsocket_protosw_size (only used once).
> M sys/netgraph/ng_gif_demux.c
> M sys/netgraph/ng_iface.c
Possibly the same for NUM_FAMILIES in these two files.
> M sys/netgraph/ng_socket.c
> M sys/netinet/in_proto.c
> M sys/netinet/tcp_syncache.c
> M sys/netinet6/in6_proto.c
> M sys/netipsec/key.c
> M sys/netipsec/keysock.c
> M sys/netnatm/natm_proto.c
> M sys/netsmb/smb_smb.c
SMB_DIALECT_MAX isn't used and should just be removed.
> M sys/nlm/nlm_prot_impl.c
Same here for version_count (only used once).
> M sys/pc98/cbus/gdc.c
> M sys/pc98/cbus/pckbd.c
> M sys/pc98/cbus/scterm-sck.c
> M sys/powerpc/powerpc/db_trace.c
> M sys/powerpc/pseries/xics.c
> M sys/security/audit/audit_bsm_klib.c
Same here for aue_open_count and aue_openat_count (each only used once).
> M sys/security/audit/bsm_errno.c
Same here for bsm_errnos_count (used twice, but both in the for loop idiom).
> M sys/security/audit/bsm_fcntl.c
Same here for bsm_fcntl_cmd_count (used twice, but both in the for loop
idiom).
> M sys/security/audit/bsm_socket_type.c
Same here for bsm_socket_types_count (used twice, but both in the for loop
idiom).
> M sys/sparc64/sparc64/db_trace.c
> M sys/sparc64/sparc64/elf_machdep.c
> M sys/sparc64/sparc64/trap.c
> M sys/vm/vm_pager.c
Same here for npagers (only used once).
> M sys/x86/isa/atpic.c
> M sys/x86/x86/identcpu.c
> M sys/x86/x86/local_apic.c
The changes overall look fine to me. I just think we should take the chance
to inline cases where the variable is only ever used once or is only used in
the for loop idiom where I think the explicit nitems() is clearer to the
reader.
--
John Baldwin
More information about the freebsd-current
mailing list