From owner-freebsd-current@freebsd.org Thu Apr 21 16:02:53 2016 Return-Path: Delivered-To: freebsd-current@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id 552ACB14D99 for ; Thu, 21 Apr 2016 16:02:53 +0000 (UTC) (envelope-from pfg@FreeBSD.org) Received: from nm23-vm1.bullet.mail.bf1.yahoo.com (nm23-vm1.bullet.mail.bf1.yahoo.com [98.139.213.141]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (Client did not present a certificate) by mx1.freebsd.org (Postfix) with ESMTPS id 1D86014ED for ; Thu, 21 Apr 2016 16:02:52 +0000 (UTC) (envelope-from pfg@FreeBSD.org) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=yahoo.com; s=s2048; t=1461254566; bh=qQ/ioJDEk/73lskgNQmCDWg7Td18dYV2oxy1tHejQjA=; h=Subject:To:References:From:Date:In-Reply-To:From:Subject; b=DBiGKLKciBTt70pzF5JogDm+Pt7U1wJyx9CAi0H1rKnClJNSVZZmoazGz388kfMyYF8hFNV9t9DP61bAzpHUgZSYc/LUvib/BKfG3sZlOJLqE41HgS9024Rq1EusTEAqxUe5m5WJDj4Bfh5/ZwXzmnmUTMOopj2YSKmXDi1X3DHXfnhZNhzJWdLvu5uuobQEuLV20qE+gtb0l2KJvV+MhVW9OmJRcovd2Q/6nq7XkjAoggmKKOJjo1N1bzR5iJGUpCPnXDp7DJkaRVFpxw9rRVP8DE/JeTvC5C8aC3bLxw36mT8KPkXyKCaZ5TmHwipPchqBmtT0eWAKGbwk7cUYHA== Received: from [98.139.170.180] by nm23.bullet.mail.bf1.yahoo.com with NNFMP; 21 Apr 2016 16:02:46 -0000 Received: from [98.139.211.193] by tm23.bullet.mail.bf1.yahoo.com with NNFMP; 21 Apr 2016 16:02:46 -0000 Received: from [127.0.0.1] by smtp202.mail.bf1.yahoo.com with NNFMP; 21 Apr 2016 16:02:46 -0000 X-Yahoo-Newman-Id: 313284.60291.bm@smtp202.mail.bf1.yahoo.com X-Yahoo-Newman-Property: ymail-3 X-YMail-OSG: zj6Cz_8VM1mdONsP4ptGZLh0AmAtuLZQSPZ9B9gGjDdw5PJ YZbGIA76Hdm2XENK1lRHKT8LZ62EmiGPy0jMqKaBD.OtkghXmSFiH1QyVA4F A7.Cdm1L1uxwJg6LFLVG.LjEEaanSD2TdVwIjQB3MqdtHGN7uuRT9GDWG1G7 KyCVpDufYNrQkvi.tZsJUHG_f9JQWnJZDBJWVZ1qyd4c20PrM8eJyhD_YCt7 FDkoIcdQ_7CV0O.jsMu9vVZPFvSHW8U0YekAsq8EjWe1nc7RxakYQ2b_iDwk _6XRR2NM9SG.3iBMLXw1EabVzXlxMMloXfvXOTlxcZW8vKoc3I1W6JVqDKIC mEMgVfO1VlBnherHgGWJyceH0WVclgL63hEUdDm7USU_FKUEKUQnWIRLqrLI k5EN0ny1wRJay5QQvSFV20KIueuZ8iHbwu9hhqCz.HTstiSS6t1L2MZdV6M. qR5E4TxMZXLu2zFYQFRd0ZmIlmE.MdOAWAEzWsggRtoslfKlARkfx764rBGc k02w3CPex.U9kwuTUKxDNsvBkQ_NjDo9d X-Yahoo-SMTP: xcjD0guswBAZaPPIbxpWwLcp9Unf Subject: Re: CFR: extend use of nitems() macro in the kernel. To: John Baldwin , freebsd-current@freebsd.org References: <57128385.6090307@FreeBSD.org> <1920453.prUgCpdaXV@ralph.baldwin.cx> From: Pedro Giffuni Message-ID: <8f87b91a-eb69-92fa-1b5b-7cf3f64b3942@FreeBSD.org> Date: Thu, 21 Apr 2016 11:02:52 -0500 User-Agent: Mozilla/5.0 (X11; FreeBSD amd64; rv:45.0) Gecko/20100101 Thunderbird/45.0 MIME-Version: 1.0 In-Reply-To: <1920453.prUgCpdaXV@ralph.baldwin.cx> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit X-BeenThere: freebsd-current@freebsd.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: Discussions about the use of FreeBSD-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 21 Apr 2016 16:02:53 -0000 On 04/18/16 13:27, John Baldwin wrote: > 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. > ... > > 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. > Huge thanks for the detailed review. I have done the cleanups related to consts and variables. I would prefer to leave the macros still there, even if they are used only once: in at least one case they actually help driver readability and may generally help understand the author's intent. It may also be that they help upstream (or downstream) maintainers. I am running a buildworld with the final nitems() changes. Regards, Pedro.