Re: [PATCH 1/1] [RFC] ethernet: Convert from tasklet to BH workqueue
Paolo, > On Wed, 2024-05-08 at 21:16 +0100, Simon Horman wrote: > > * As this patch seems to involve many non-trivial changes > > it seems to me that it would be best to break it up somehow. > > To allow proper review. > > I would like to stress this latest point: it looks like the changes to > all the drivers are completely independent. If so, you have to break > the series on a per driver basis. Since the total number of patch will > be higher then 15 (maximum size allowed on netdev) you will have to > split this in several smaller series. > Right, it's a valid point. Per-driver might not work. Depending on the driver and changes, I will try and make it an independent series. > Beyond making the change reviewable, it will allow eventually reverting > the changes individually, should that cause any regressions. > Thank you, I understand the concern here. Will work on it in v2. Thank you very much for your time and suggestions. - Allen
Re: [PATCH 1/1] [RFC] ethernet: Convert from tasklet to BH workqueue
> > > On Tue, May 07, 2024 at 07:01:11PM +, Allen Pais wrote: > > > > The only generic interface to execute asynchronously in the BH context > > > > is > > > > tasklet; however, it's marked deprecated and has some design flaws. To > > > > replace tasklets, BH workqueue support was recently added. A BH > > > > workqueue > > > > behaves similarly to regular workqueues except that the queued work > > > > items > > > > are executed in the BH context. > > > > > > > > This patch converts drivers/ethernet/* from tasklet to BH workqueue. > > > > > > I doubt you're going to get many comments on this patch, being so large > > > and spread across all drivers. I'm not going to bother trying to edit > > > this down to something more sensible, I'll just plonk my comment here. > > > > > > For the mvpp2 driver, you're only updating a comment - and looking at > > > it, the comment no longer reflects the code. It doesn't make use of > > > tasklets at all. That makes the comment wrong whether or not it's > > > updated. So I suggest rather than doing a search and replace for > > > "tasklet" to "BH blahblah" (sorry, I don't remember what you replaced > > > it with) just get rid of that bit of the comment. > > > > > > > Thank you Russell. > > > > I will get rid of the comment. If it helps, I can create a patch for each > > driver. We did that in the past, with this series, I thought it would be > > easier to apply one patch. > > Hi Allen and Russell, > > My 2c worth: > > * In general non bug-fix patches for networking code should be targeted at > net-next. This means that they should include net-next in the subject, > and be based on that tree. > > Subject: [PATCH net-next] ... > > * This series does not appear to apply to net-next > > * This series appears to depend on code which is not present in net-next. > f.e. disable_work_sync > > * The Infiniband patches should probably be submitted separately > to the relevant maintainers > > * As this patch seems to involve many non-trivial changes > it seems to me that it would be best to break it up somehow. > To allow proper review. > > * Patch-sets for net-next should be limited to 15 patches, > so perhaps multiple sequential batches would be a way forwards. > > Link: https://docs.kernel.org/process/maintainer-netdev.html Thank you very much for taking the time to write back. Since the patches that are necessary for this series are not in net-next, I could not target net-next. I will wait for the patches to land in net-next, and the v2 will be broken into multiple smaller sets(per driver). Thanks. Allen
Re: [PATCH 1/1] [RFC] ethernet: Convert from tasklet to BH workqueue
On Wed, 2024-05-08 at 21:16 +0100, Simon Horman wrote: > * As this patch seems to involve many non-trivial changes > it seems to me that it would be best to break it up somehow. > To allow proper review. I would like to stress this latest point: it looks like the changes to all the drivers are completely independent. If so, you have to break the series on a per driver basis. Since the total number of patch will be higher then 15 (maximum size allowed on netdev) you will have to split this in several smaller series. Beyond making the change reviewable, it will allow eventually reverting the changes individually, should that cause any regressions. Thanks, Paolo
Re: [PATCH 1/1] [RFC] ethernet: Convert from tasklet to BH workqueue
On Tue, May 07, 2024 at 12:27:10PM -0700, Allen wrote: > On Tue, May 7, 2024 at 12:23 PM Russell King (Oracle) > wrote: > > > > On Tue, May 07, 2024 at 07:01:11PM +, Allen Pais wrote: > > > The only generic interface to execute asynchronously in the BH context is > > > tasklet; however, it's marked deprecated and has some design flaws. To > > > replace tasklets, BH workqueue support was recently added. A BH workqueue > > > behaves similarly to regular workqueues except that the queued work items > > > are executed in the BH context. > > > > > > This patch converts drivers/ethernet/* from tasklet to BH workqueue. > > > > I doubt you're going to get many comments on this patch, being so large > > and spread across all drivers. I'm not going to bother trying to edit > > this down to something more sensible, I'll just plonk my comment here. > > > > For the mvpp2 driver, you're only updating a comment - and looking at > > it, the comment no longer reflects the code. It doesn't make use of > > tasklets at all. That makes the comment wrong whether or not it's > > updated. So I suggest rather than doing a search and replace for > > "tasklet" to "BH blahblah" (sorry, I don't remember what you replaced > > it with) just get rid of that bit of the comment. > > > > Thank you Russell. > > I will get rid of the comment. If it helps, I can create a patch for each > driver. We did that in the past, with this series, I thought it would be > easier to apply one patch. Hi Allen and Russell, My 2c worth: * In general non bug-fix patches for networking code should be targeted at net-next. This means that they should include net-next in the subject, and be based on that tree. Subject: [PATCH net-next] ... * This series does not appear to apply to net-next * This series appears to depend on code which is not present in net-next. f.e. disable_work_sync * The Infiniband patches should probably be submitted separately to the relevant maintainers * As this patch seems to involve many non-trivial changes it seems to me that it would be best to break it up somehow. To allow proper review. * Patch-sets for net-next should be limited to 15 patches, so perhaps multiple sequential batches would be a way forwards. Link: https://docs.kernel.org/process/maintainer-netdev.html
Re: [PATCH 1/1] [RFC] ethernet: Convert from tasklet to BH workqueue
On Tue, May 07, 2024 at 07:01:11PM +, Allen Pais wrote: > The only generic interface to execute asynchronously in the BH context is > tasklet; however, it's marked deprecated and has some design flaws. To > replace tasklets, BH workqueue support was recently added. A BH workqueue > behaves similarly to regular workqueues except that the queued work items > are executed in the BH context. > > This patch converts drivers/ethernet/* from tasklet to BH workqueue. I doubt you're going to get many comments on this patch, being so large and spread across all drivers. I'm not going to bother trying to edit this down to something more sensible, I'll just plonk my comment here. For the mvpp2 driver, you're only updating a comment - and looking at it, the comment no longer reflects the code. It doesn't make use of tasklets at all. That makes the comment wrong whether or not it's updated. So I suggest rather than doing a search and replace for "tasklet" to "BH blahblah" (sorry, I don't remember what you replaced it with) just get rid of that bit of the comment. Thanks. -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
Re: [PATCH 1/1] [RFC] ethernet: Convert from tasklet to BH workqueue
On Tue, May 7, 2024 at 12:23 PM Russell King (Oracle) wrote: > > On Tue, May 07, 2024 at 07:01:11PM +, Allen Pais wrote: > > The only generic interface to execute asynchronously in the BH context is > > tasklet; however, it's marked deprecated and has some design flaws. To > > replace tasklets, BH workqueue support was recently added. A BH workqueue > > behaves similarly to regular workqueues except that the queued work items > > are executed in the BH context. > > > > This patch converts drivers/ethernet/* from tasklet to BH workqueue. > > I doubt you're going to get many comments on this patch, being so large > and spread across all drivers. I'm not going to bother trying to edit > this down to something more sensible, I'll just plonk my comment here. > > For the mvpp2 driver, you're only updating a comment - and looking at > it, the comment no longer reflects the code. It doesn't make use of > tasklets at all. That makes the comment wrong whether or not it's > updated. So I suggest rather than doing a search and replace for > "tasklet" to "BH blahblah" (sorry, I don't remember what you replaced > it with) just get rid of that bit of the comment. > Thank you Russell. I will get rid of the comment. If it helps, I can create a patch for each driver. We did that in the past, with this series, I thought it would be easier to apply one patch. Thanks, - Allen
[PATCH 1/1] [RFC] ethernet: Convert from tasklet to BH workqueue
The only generic interface to execute asynchronously in the BH context is tasklet; however, it's marked deprecated and has some design flaws. To replace tasklets, BH workqueue support was recently added. A BH workqueue behaves similarly to regular workqueues except that the queued work items are executed in the BH context. This patch converts drivers/ethernet/* from tasklet to BH workqueue. Based on the work done by Tejun Heo Branch: https://git.kernel.org/pub/scm/linux/kernel/git/tj/wq.git disable_work-v1 Signed-off-by: Allen Pais --- drivers/infiniband/hw/mlx4/cq.c | 2 +- drivers/infiniband/hw/mlx5/cq.c | 2 +- drivers/net/ethernet/alteon/acenic.c | 26 +++ drivers/net/ethernet/alteon/acenic.h | 7 +- drivers/net/ethernet/amd/xgbe/xgbe-drv.c | 30 drivers/net/ethernet/amd/xgbe/xgbe-i2c.c | 16 ++--- drivers/net/ethernet/amd/xgbe/xgbe-mdio.c | 16 ++--- drivers/net/ethernet/amd/xgbe/xgbe-pci.c | 4 +- drivers/net/ethernet/amd/xgbe/xgbe.h | 11 +-- drivers/net/ethernet/broadcom/cnic.c | 19 ++--- drivers/net/ethernet/broadcom/cnic.h | 2 +- drivers/net/ethernet/cadence/macb.h | 3 +- drivers/net/ethernet/cadence/macb_main.c | 10 +-- .../net/ethernet/cavium/liquidio/lio_core.c | 4 +- .../net/ethernet/cavium/liquidio/lio_main.c | 25 +++ .../ethernet/cavium/liquidio/lio_vf_main.c| 10 +-- .../ethernet/cavium/liquidio/octeon_droq.c| 4 +- .../ethernet/cavium/liquidio/octeon_main.h| 5 +- .../net/ethernet/cavium/octeon/octeon_mgmt.c | 12 ++-- drivers/net/ethernet/cavium/thunder/nic.h | 5 +- .../net/ethernet/cavium/thunder/nicvf_main.c | 24 +++ .../ethernet/cavium/thunder/nicvf_queues.c| 5 +- .../ethernet/cavium/thunder/nicvf_queues.h| 3 +- drivers/net/ethernet/chelsio/cxgb/sge.c | 19 ++--- drivers/net/ethernet/chelsio/cxgb4/cxgb4.h| 9 +-- .../net/ethernet/chelsio/cxgb4/cxgb4_main.c | 2 +- .../ethernet/chelsio/cxgb4/cxgb4_tc_mqprio.c | 4 +- .../net/ethernet/chelsio/cxgb4/cxgb4_uld.c| 2 +- drivers/net/ethernet/chelsio/cxgb4/sge.c | 41 +-- drivers/net/ethernet/chelsio/cxgb4vf/sge.c| 6 +- drivers/net/ethernet/dlink/sundance.c | 41 +-- .../net/ethernet/huawei/hinic/hinic_hw_cmdq.c | 2 +- .../net/ethernet/huawei/hinic/hinic_hw_eqs.c | 17 +++-- .../net/ethernet/huawei/hinic/hinic_hw_eqs.h | 2 +- drivers/net/ethernet/ibm/ehea/ehea.h | 3 +- drivers/net/ethernet/ibm/ehea/ehea_main.c | 14 ++-- drivers/net/ethernet/ibm/ibmvnic.c| 24 +++ drivers/net/ethernet/ibm/ibmvnic.h| 2 +- drivers/net/ethernet/jme.c| 72 +-- drivers/net/ethernet/jme.h| 9 +-- .../net/ethernet/marvell/mvpp2/mvpp2_main.c | 2 +- drivers/net/ethernet/marvell/skge.c | 12 ++-- drivers/net/ethernet/marvell/skge.h | 3 +- drivers/net/ethernet/mediatek/mtk_wed_wo.c| 12 ++-- drivers/net/ethernet/mediatek/mtk_wed_wo.h| 3 +- drivers/net/ethernet/mellanox/mlx4/cq.c | 42 +-- drivers/net/ethernet/mellanox/mlx4/eq.c | 10 +-- drivers/net/ethernet/mellanox/mlx4/mlx4.h | 11 +-- drivers/net/ethernet/mellanox/mlx5/core/cq.c | 38 +- drivers/net/ethernet/mellanox/mlx5/core/eq.c | 12 ++-- .../ethernet/mellanox/mlx5/core/fpga/conn.c | 15 ++-- .../ethernet/mellanox/mlx5/core/fpga/conn.h | 3 +- .../net/ethernet/mellanox/mlx5/core/lib/eq.h | 11 +-- drivers/net/ethernet/mellanox/mlxsw/pci.c | 29 drivers/net/ethernet/micrel/ks8842.c | 29 drivers/net/ethernet/micrel/ksz884x.c | 37 +- drivers/net/ethernet/microchip/lan743x_ptp.c | 2 +- drivers/net/ethernet/natsemi/ns83820.c| 10 +-- drivers/net/ethernet/netronome/nfp/nfd3/dp.c | 7 +- .../net/ethernet/netronome/nfp/nfd3/nfd3.h| 2 +- drivers/net/ethernet/netronome/nfp/nfdk/dp.c | 6 +- .../net/ethernet/netronome/nfp/nfdk/nfdk.h| 3 +- drivers/net/ethernet/netronome/nfp/nfp_net.h | 4 +- .../ethernet/netronome/nfp/nfp_net_common.c | 12 ++-- .../net/ethernet/netronome/nfp/nfp_net_dp.h | 4 +- drivers/net/ethernet/ni/nixge.c | 19 ++--- drivers/net/ethernet/qlogic/qed/qed.h | 2 +- drivers/net/ethernet/qlogic/qed/qed_int.c | 6 +- drivers/net/ethernet/qlogic/qed/qed_int.h | 4 +- drivers/net/ethernet/qlogic/qed/qed_main.c| 20 +++--- drivers/net/ethernet/sfc/falcon/farch.c | 4 +- drivers/net/ethernet/sfc/falcon/net_driver.h | 2 +- drivers/net/ethernet/sfc/falcon/selftest.c| 2 +- drivers/net/ethernet/sfc/net_driver.h | 2 +- drivers/net/ethernet/sfc/selftest.c | 2 +- drivers/net/ethernet/sfc/siena/farch.c| 4 +- drivers/net/ethernet/sfc/siena/net_driver.h | 2 +- drivers/net/ethernet/sfc/siena/selftest.c | 2 +-
