| Message ID | 20240730183403.4176544-15-allen.lkml@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | ethernet: Convert from tasklet to BH workqueue | expand |
> - * Called only from mvpp2_txq_done(), called from mvpp2_tx() > - * (migration disabled) and from the TX completion tasklet (migration > - * disabled) so using smp_processor_id() is OK. > + * Called only from mvpp2_txq_done(). > + * > + * Historically, this function was invoked directly from mvpp2_tx() > + * (with migration disabled) and from the bottom half workqueue. > + * Verify that the use of smp_processor_id() is still appropriate > + * considering the current bottom half workqueue implementation. What does this mean? You want somebody else to verify this? You are potentially breaking this driver? Andrew
On Tue, Jul 30, 2024 at 10:39:51PM +0200, Andrew Lunn wrote: > > - * Called only from mvpp2_txq_done(), called from mvpp2_tx() > > - * (migration disabled) and from the TX completion tasklet (migration > > - * disabled) so using smp_processor_id() is OK. > > + * Called only from mvpp2_txq_done(). > > + * > > + * Historically, this function was invoked directly from mvpp2_tx() > > + * (with migration disabled) and from the bottom half workqueue. > > + * Verify that the use of smp_processor_id() is still appropriate > > + * considering the current bottom half workqueue implementation. > > What does this mean? You want somebody else to verify this? You are > potentially breaking this driver? I don't see how, the only thing that's changing in mvpp2 seems to be an outdated comment that happens to mention a tasklet, but the driver doesn't use tasklets. Let's look at the original comment which claims what the call sites are: static void mvpp2_txq_done(struct mvpp2_port *port, struct mvpp2_tx_queue *txq, struct mvpp2_txq_pcpu *txq_pcpu) { ... tx_done = mvpp2_txq_sent_desc_proc(port, txq); and that is it. _This_ function is called from several places: mvpp2_tx_done() mvpp2_xdp_finish_tx() mvpp2_tx() So I suppose that the original comment was referring to the mvpp2_tx() -> mvpp2_txq_done() -> mvpp2_txq_sent_desc_proc() call path, and the others were added over time. mvpp2_tx_done() is called from mvpp2_hr_timer_cb(), and yes, back in the distant history there was a tasklet here - see: ecb9f80db23a net/mvpp2: Replace tasklet with softirq hrtimer So, the comment referring to a tasklet was left over from that commit and never fixed up. Given this, I don't think the new paragraph starting "Historically" is correct (or even relevant) as I think it misinterprets the original comment - and "this function" is ambiguous in it, but either way its still wrong. If we assume that "this function" refers to the one below the comment, then this has never been called directly from mvpp2_tx() nor the tasklet, and talking about a bottom half workqueue makes no sense because "historically" it's never been called from a bottom half workqueue. If we assume that "this function" refers to mvpp2_txq_done(), then it's not historical that this was called from mvpp2_tx(), because it still is today. And the bit about being called from a bottom half workqueue is still false. Given that bottom half workqueues have absolutely nothing to do with this code path, the sentence beginning with "Verify" seems totally irrelevant (at least to me.) So, I think I've comprehensively ripped the new comment to shreds. It would be far better to leave the driver alone and not change the comment despite it incorrectly referring to a tasklet that has already been eliminated (and at least was historically correct), rather than the new comment which just seems wrong.
> > - * Called only from mvpp2_txq_done(), called from mvpp2_tx() > > - * (migration disabled) and from the TX completion tasklet (migration > > - * disabled) so using smp_processor_id() is OK. > > + * Called only from mvpp2_txq_done(). > > + * > > + * Historically, this function was invoked directly from mvpp2_tx() > > + * (with migration disabled) and from the bottom half workqueue. > > + * Verify that the use of smp_processor_id() is still appropriate > > + * considering the current bottom half workqueue implementation. > > What does this mean? You want somebody else to verify this? You are > potentially breaking this driver? > Thanks for providing the review. Apologies for not having worded this correctly. Russel did ask me to leave it as it was when I first sent out the series. Perhaps I should do so. Kindly advice. Thanks, Allen
> On Tue, Jul 30, 2024 at 10:39:51PM +0200, Andrew Lunn wrote: > > > - * Called only from mvpp2_txq_done(), called from mvpp2_tx() > > > - * (migration disabled) and from the TX completion tasklet (migration > > > - * disabled) so using smp_processor_id() is OK. > > > + * Called only from mvpp2_txq_done(). > > > + * > > > + * Historically, this function was invoked directly from mvpp2_tx() > > > + * (with migration disabled) and from the bottom half workqueue. > > > + * Verify that the use of smp_processor_id() is still appropriate > > > + * considering the current bottom half workqueue implementation. > > > > What does this mean? You want somebody else to verify this? You are > > potentially breaking this driver? > > I don't see how, the only thing that's changing in mvpp2 seems to be > an outdated comment that happens to mention a tasklet, but the > driver doesn't use tasklets. > > Let's look at the original comment which claims what the call sites > are: > > static void mvpp2_txq_done(struct mvpp2_port *port, struct mvpp2_tx_queue *txq, > struct mvpp2_txq_pcpu *txq_pcpu) > { > ... > tx_done = mvpp2_txq_sent_desc_proc(port, txq); > > and that is it. _This_ function is called from several places: > > mvpp2_tx_done() > mvpp2_xdp_finish_tx() > mvpp2_tx() > > So I suppose that the original comment was referring to the > mvpp2_tx() -> mvpp2_txq_done() -> mvpp2_txq_sent_desc_proc() call path, > and the others were added over time. You are right, I should have checked the calls before modifying the comment. > > mvpp2_tx_done() is called from mvpp2_hr_timer_cb(), and yes, back in > the distant history there was a tasklet here - see: > > ecb9f80db23a net/mvpp2: Replace tasklet with softirq hrtimer I missed this bit completely, "historically...." is completely wrong and misleading. > > So, the comment referring to a tasklet was left over from that commit > and never fixed up. > > Given this, I don't think the new paragraph starting "Historically" > is correct (or even relevant) as I think it misinterprets the original > comment - and "this function" is ambiguous in it, but either way its > still wrong. > > If we assume that "this function" refers to the one below the comment, > then this has never been called directly from mvpp2_tx() nor the > tasklet, and talking about a bottom half workqueue makes no sense > because "historically" it's never been called from a bottom half > workqueue. > > If we assume that "this function" refers to mvpp2_txq_done(), then > it's not historical that this was called from mvpp2_tx(), because it > still is today. And the bit about being called from a bottom half > workqueue is still false. > > Given that bottom half workqueues have absolutely nothing to do with > this code path, the sentence beginning with "Verify" seems totally > irrelevant (at least to me.) > > So, I think I've comprehensively ripped the new comment to shreds. > It would be far better to leave the driver alone and not change the > comment despite it incorrectly referring to a tasklet that has > already been eliminated (and at least was historically correct), > rather than the new comment which just seems wrong. I am open to leaving the comment as is or completely removing it. I am open to suggestions, please let me know what would be the correct thing to do. Thanks for taking the time out to review and write back. - Allen > > -- > RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ > FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
diff --git a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c index 8c45ad983abc..adffbbd20962 100644 --- a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c +++ b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c @@ -2628,9 +2628,12 @@ static u32 mvpp2_txq_desc_csum(int l3_offs, __be16 l3_proto, * The number of sent descriptors is returned. * Per-thread access * - * Called only from mvpp2_txq_done(), called from mvpp2_tx() - * (migration disabled) and from the TX completion tasklet (migration - * disabled) so using smp_processor_id() is OK. + * Called only from mvpp2_txq_done(). + * + * Historically, this function was invoked directly from mvpp2_tx() + * (with migration disabled) and from the bottom half workqueue. + * Verify that the use of smp_processor_id() is still appropriate + * considering the current bottom half workqueue implementation. */ static inline int mvpp2_txq_sent_desc_proc(struct mvpp2_port *port, struct mvpp2_tx_queue *txq) diff --git a/drivers/net/ethernet/marvell/skge.c b/drivers/net/ethernet/marvell/skge.c index fcfb34561882..4448af079447 100644 --- a/drivers/net/ethernet/marvell/skge.c +++ b/drivers/net/ethernet/marvell/skge.c @@ -3342,13 +3342,13 @@ static void skge_error_irq(struct skge_hw *hw) } /* - * Interrupt from PHY are handled in tasklet (softirq) + * Interrupt from PHY are handled in bh work (softirq) * because accessing phy registers requires spin wait which might * cause excess interrupt latency. */ -static void skge_extirq(struct tasklet_struct *t) +static void skge_extirq(struct work_struct *work) { - struct skge_hw *hw = from_tasklet(hw, t, phy_task); + struct skge_hw *hw = from_work(hw, work, phy_bh_work); int port; for (port = 0; port < hw->ports; port++) { @@ -3389,7 +3389,7 @@ static irqreturn_t skge_intr(int irq, void *dev_id) status &= hw->intr_mask; if (status & IS_EXT_REG) { hw->intr_mask &= ~IS_EXT_REG; - tasklet_schedule(&hw->phy_task); + queue_work(system_bh_wq, &hw->phy_bh_work); } if (status & (IS_XA1_F|IS_R1_F)) { @@ -3937,7 +3937,7 @@ static int skge_probe(struct pci_dev *pdev, const struct pci_device_id *ent) hw->pdev = pdev; spin_lock_init(&hw->hw_lock); spin_lock_init(&hw->phy_lock); - tasklet_setup(&hw->phy_task, skge_extirq); + INIT_WORK(&hw->phy_bh_work, skge_extirq); hw->regs = ioremap(pci_resource_start(pdev, 0), 0x4000); if (!hw->regs) { @@ -4035,7 +4035,7 @@ static void skge_remove(struct pci_dev *pdev) dev0 = hw->dev[0]; unregister_netdev(dev0); - tasklet_kill(&hw->phy_task); + cancel_work_sync(&hw->phy_bh_work); spin_lock_irq(&hw->hw_lock); hw->intr_mask = 0; diff --git a/drivers/net/ethernet/marvell/skge.h b/drivers/net/ethernet/marvell/skge.h index f72217348eb4..0cf77f4b1c57 100644 --- a/drivers/net/ethernet/marvell/skge.h +++ b/drivers/net/ethernet/marvell/skge.h @@ -5,6 +5,7 @@ #ifndef _SKGE_H #define _SKGE_H #include <linux/interrupt.h> +#include <linux/workqueue.h> /* PCI config registers */ #define PCI_DEV_REG1 0x40 @@ -2418,7 +2419,7 @@ struct skge_hw { u32 ram_offset; u16 phy_addr; spinlock_t phy_lock; - struct tasklet_struct phy_task; + struct work_struct phy_bh_work; char irq_name[]; /* skge@pci:000:04:00.0 */ };
Migrate tasklet APIs to the new bottom half workqueue mechanism. It replaces all occurrences of tasklet usage with the appropriate workqueue APIs throughout the marvell drivers. This transition ensures compatibility with the latest design and enhances performance. Signed-off-by: Allen Pais <allen.lkml@gmail.com> --- drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c | 9 ++++++--- drivers/net/ethernet/marvell/skge.c | 12 ++++++------ drivers/net/ethernet/marvell/skge.h | 3 ++- 3 files changed, 14 insertions(+), 10 deletions(-)