| Message ID | 20210412003802.51613-1-mcroce@linux.microsoft.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | introduce skb_for_each_frag() | expand |
From: Matteo Croce > Sent: 12 April 2021 01:38 > > Introduce skb_for_each_frag, an helper macro to iterate over the SKB frags. The real question is why, the change is: - for (i = 0; i < skb_shinfo(skb)->nr_frags; i++) { + skb_for_each_frag(skb, i) { The existing code isn't complicated or obscure and 'does what it says on the tin'. The 'helper' requires you go and look up its definition to see what it is really doing. Unless you have a cunning plan to change the definition there is zero point. A more interesting change would be something that generated: unsigned int nr_frags = skb_shinfo(skb)->nr_frags; for (i = 0; i < nr_frags; i++) { since that will run faster for most loops. But that is ~impossible to do since you can't declare variables inside the (...) that are scoped to the loop. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
On Tue, Apr 13, 2021 at 9:53 AM David Laight <David.Laight@aculab.com> wrote: > > From: Matteo Croce > > Sent: 12 April 2021 01:38 > > > > Introduce skb_for_each_frag, an helper macro to iterate over the SKB frags. > > The real question is why, the change is: > > - for (i = 0; i < skb_shinfo(skb)->nr_frags; i++) { > + skb_for_each_frag(skb, i) { > > The existing code isn't complicated or obscure and 'does what it > says on the tin'. > The 'helper' requires you go and look up its definition to see > what it is really doing. > > Unless you have a cunning plan to change the definition > there is zero point. > > A more interesting change would be something that generated: > unsigned int nr_frags = skb_shinfo(skb)->nr_frags; > for (i = 0; i < nr_frags; i++) { > since that will run faster for most loops. > But that is ~impossible to do since you can't declare > variables inside the (...) that are scoped to the loop. > I don't know how to do it with C90. It would be nice to have a switch to just allow declaration of variables inside the (...) instead of enabling the full C99 language which, as Linus said[1], allows the insane mixing of variables and code. [1] https://lore.kernel.org/lkml/CA+55aFzs=DuYibWYMUFiU_R1aJHAr-8hpQhWLew8R5q4nCDraQ@mail.gmail.com/
From: Matteo Croce > Sent: 16 April 2021 23:44 ... > > A more interesting change would be something that generated: > > unsigned int nr_frags = skb_shinfo(skb)->nr_frags; > > for (i = 0; i < nr_frags; i++) { > > since that will run faster for most loops. > > But that is ~impossible to do since you can't declare > > variables inside the (...) that are scoped to the loop. > > > > I don't know how to do it with C90. > It would be nice to have a switch to just allow declaration of > variables inside the (...) instead of enabling the full C99 language > which, as Linus said[1], allows the insane mixing of variables and > code. > > [1] https://lore.kernel.org/lkml/CA+55aFzs=DuYibWYMUFiU_R1aJHAr-8hpQhWLew8R5q4nCDraQ@mail.gmail.com/ Quoting Linus: > I *like* getting warnings for confused people who start introducing > variables in the middle of blocks of code. That's not well-contained > like the loop variable. The really stupid part of C99 is that such variables can alias ones in an outer block. Aliased definitions are bad enough at the best of times. Makes it very easy to miss the correct definition when reading code. I much prefer local variables to either function scope or be defined for very local use at the top of a very small block. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
From: Matteo Croce <mcroce@microsoft.com> Introduce skb_for_each_frag, an helper macro to iterate over the SKB frags. First patch introduces the helper, the second one is generated with coccinelle and uses the macro where possible. Last one is a chunk which have to be applied by hand. The second patch raises some checkpatch.pl warnings because part of net/tls/tls_sw.c is indented with spaces. Build tested with an allmodconfig and a test run. v1 -> v2: - don't replace code where a local variable holds a cached value for skb_shinfo(skb)->nr_frags Matteo Croce (3): skbuff: add helper to walk over the fraglist net: use skb_for_each_frag() helper where possible net: use skb_for_each_frag() in illegal_highdma() drivers/atm/he.c | 2 +- drivers/hsi/clients/ssi_protocol.c | 2 +- drivers/infiniband/hw/hfi1/ipoib_tx.c | 2 +- drivers/infiniband/hw/hfi1/vnic_sdma.c | 2 +- drivers/infiniband/ulp/ipoib/ipoib_ib.c | 4 +-- drivers/net/ethernet/3com/3c59x.c | 2 +- drivers/net/ethernet/3com/typhoon.c | 2 +- drivers/net/ethernet/adaptec/starfire.c | 2 +- drivers/net/ethernet/aeroflex/greth.c | 2 +- drivers/net/ethernet/alteon/acenic.c | 2 +- drivers/net/ethernet/amd/xgbe/xgbe-desc.c | 2 +- drivers/net/ethernet/amd/xgbe/xgbe-drv.c | 2 +- .../net/ethernet/apm/xgene/xgene_enet_main.c | 2 +- drivers/net/ethernet/atheros/alx/main.c | 2 +- .../net/ethernet/atheros/atl1e/atl1e_main.c | 2 +- .../net/ethernet/broadcom/bnx2x/bnx2x_cmn.c | 2 +- drivers/net/ethernet/broadcom/tg3.c | 2 +- .../ethernet/cavium/thunder/nicvf_queues.c | 2 +- drivers/net/ethernet/chelsio/cxgb3/sge.c | 2 +- drivers/net/ethernet/emulex/benet/be_main.c | 2 +- .../net/ethernet/freescale/dpaa/dpaa_eth.c | 2 +- drivers/net/ethernet/freescale/gianfar.c | 3 +- drivers/net/ethernet/google/gve/gve_tx.c | 2 +- drivers/net/ethernet/hisilicon/hix5hd2_gmac.c | 4 +-- .../net/ethernet/hisilicon/hns3/hns3_enet.c | 4 +-- drivers/net/ethernet/huawei/hinic/hinic_rx.c | 2 +- drivers/net/ethernet/huawei/hinic/hinic_tx.c | 4 +-- drivers/net/ethernet/ibm/ibmveth.c | 2 +- drivers/net/ethernet/ibm/ibmvnic.c | 2 +- drivers/net/ethernet/intel/fm10k/fm10k_main.c | 2 +- drivers/net/ethernet/intel/igb/igb_main.c | 2 +- drivers/net/ethernet/intel/igbvf/netdev.c | 2 +- drivers/net/ethernet/intel/igc/igc_main.c | 2 +- drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 2 +- .../net/ethernet/intel/ixgbevf/ixgbevf_main.c | 2 +- drivers/net/ethernet/marvell/mv643xx_eth.c | 2 +- .../net/ethernet/marvell/mvpp2/mvpp2_main.c | 2 +- drivers/net/ethernet/marvell/skge.c | 2 +- drivers/net/ethernet/marvell/sky2.c | 8 ++--- drivers/net/ethernet/mediatek/mtk_eth_soc.c | 2 +- .../net/ethernet/mellanox/mlx5/core/en_tx.c | 2 +- drivers/net/ethernet/mellanox/mlxsw/pci.c | 2 +- drivers/net/ethernet/realtek/8139cp.c | 2 +- drivers/net/ethernet/realtek/r8169_main.c | 2 +- drivers/net/ethernet/rocker/rocker_main.c | 2 +- drivers/net/ethernet/sfc/tx.c | 2 +- drivers/net/ethernet/sun/niu.c | 4 +-- drivers/net/ethernet/sun/sungem.c | 2 +- drivers/net/ethernet/sun/sunhme.c | 2 +- drivers/net/ethernet/sun/sunvnet_common.c | 4 +-- .../net/ethernet/synopsys/dwc-xlgmac-desc.c | 2 +- .../net/ethernet/synopsys/dwc-xlgmac-net.c | 2 +- drivers/net/ethernet/ti/am65-cpsw-nuss.c | 2 +- drivers/net/ethernet/ti/netcp_core.c | 2 +- drivers/net/ethernet/via/via-velocity.c | 2 +- drivers/net/usb/usbnet.c | 2 +- drivers/net/vmxnet3/vmxnet3_drv.c | 4 +-- drivers/net/wireless/intel/iwlwifi/pcie/tx.c | 2 +- drivers/net/wireless/intel/iwlwifi/queue/tx.c | 2 +- drivers/net/xen-netback/netback.c | 2 +- drivers/net/xen-netfront.c | 2 +- drivers/s390/net/qeth_core_main.c | 4 +-- drivers/scsi/fcoe/fcoe_transport.c | 2 +- drivers/staging/octeon/ethernet-tx.c | 2 +- drivers/target/iscsi/cxgbit/cxgbit_target.c | 4 +-- include/linux/skbuff.h | 4 +++ net/appletalk/ddp.c | 2 +- net/core/datagram.c | 4 +-- net/core/dev.c | 2 +- net/core/skbuff.c | 32 +++++++++---------- net/ipv4/inet_fragment.c | 2 +- net/ipv4/tcp.c | 2 +- net/ipv4/tcp_output.c | 2 +- net/iucv/af_iucv.c | 4 +-- net/kcm/kcmsock.c | 3 +- net/tls/tls_sw.c | 2 +- 76 files changed, 108 insertions(+), 106 deletions(-)