| Message ID | 3ef7166d-ce47-e24c-1df4-bb76a39c96c3@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | r8169: use new macros from netdev_queues.h | expand |
| Context | Check | Description |
|---|---|---|
| netdev/series_format | success | Posting correctly formatted |
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/fixes_present | success | Fixes tag not required for -next series |
| netdev/header_inline | success | No static functions without inline keyword in header files |
| netdev/build_32bit | success | Errors and warnings before: 18 this patch: 18 |
| netdev/cc_maintainers | success | CCed 7 of 7 maintainers |
| netdev/build_clang | success | Errors and warnings before: 18 this patch: 18 |
| netdev/verify_signedoff | success | Signed-off-by tag matches author and committer |
| netdev/deprecated_api | success | None detected |
| netdev/check_selftest | success | No net selftest shell script |
| netdev/verify_fixes | success | No Fixes tag |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 18 this patch: 18 |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 24 lines checked |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/source_inline | success | Was 0 now: 0 |
On 4/13/2023 12:16 PM, Heiner Kallweit wrote: > Use new net core macro netif_subqueue_completed_wake to simplify > the code of the tx cleanup path. > > Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> > --- > drivers/net/ethernet/realtek/r8169_main.c | 16 ++++------------ > 1 file changed, 4 insertions(+), 12 deletions(-) > > diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c > index 3f0b78fd9..5cfdb60ab 100644 > --- a/drivers/net/ethernet/realtek/r8169_main.c > +++ b/drivers/net/ethernet/realtek/r8169_main.c > @@ -4372,20 +4372,12 @@ static void rtl_tx(struct net_device *dev, struct rtl8169_private *tp, > } > > if (tp->dirty_tx != dirty_tx) { > - netdev_completed_queue(dev, pkts_compl, bytes_compl); > dev_sw_netstats_tx_add(dev, pkts_compl, bytes_compl); > + WRITE_ONCE(tp->dirty_tx, dirty_tx); > > - /* Sync with rtl8169_start_xmit: > - * - publish dirty_tx ring index (write barrier) > - * - refresh cur_tx ring index and queue status (read barrier) > - * May the current thread miss the stopped queue condition, > - * a racing xmit thread can only have a right view of the > - * ring status. > - */ > - smp_store_mb(tp->dirty_tx, dirty_tx); We used to use a smp_store_mb here, but now its safe to just WRITE_ONCE? Assuming that's correct: Reviewed-by: Jacob Keller <jacob.e.keller@intel.com> > - if (netif_queue_stopped(dev) && > - rtl_tx_slots_avail(tp) >= R8169_TX_START_THRS) > - netif_wake_queue(dev); > + netif_subqueue_completed_wake(dev, 0, pkts_compl, bytes_compl, > + rtl_tx_slots_avail(tp), > + R8169_TX_START_THRS); > /* > * 8168 hack: TxPoll requests are lost when the Tx packets are > * too close. Let's kick an extra TxPoll request when a burst
On 13.04.2023 21:25, Jacob Keller wrote: > > > On 4/13/2023 12:16 PM, Heiner Kallweit wrote: >> Use new net core macro netif_subqueue_completed_wake to simplify >> the code of the tx cleanup path. >> >> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> >> --- >> drivers/net/ethernet/realtek/r8169_main.c | 16 ++++------------ >> 1 file changed, 4 insertions(+), 12 deletions(-) >> >> diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c >> index 3f0b78fd9..5cfdb60ab 100644 >> --- a/drivers/net/ethernet/realtek/r8169_main.c >> +++ b/drivers/net/ethernet/realtek/r8169_main.c >> @@ -4372,20 +4372,12 @@ static void rtl_tx(struct net_device *dev, struct rtl8169_private *tp, >> } >> >> if (tp->dirty_tx != dirty_tx) { >> - netdev_completed_queue(dev, pkts_compl, bytes_compl); >> dev_sw_netstats_tx_add(dev, pkts_compl, bytes_compl); >> + WRITE_ONCE(tp->dirty_tx, dirty_tx); >> >> - /* Sync with rtl8169_start_xmit: >> - * - publish dirty_tx ring index (write barrier) >> - * - refresh cur_tx ring index and queue status (read barrier) >> - * May the current thread miss the stopped queue condition, >> - * a racing xmit thread can only have a right view of the >> - * ring status. >> - */ >> - smp_store_mb(tp->dirty_tx, dirty_tx); > > > We used to use a smp_store_mb here, but now its safe to just WRITE_ONCE? > Assuming that's correct: > netdev_completed_queue() has a smp_mb() and is called from netif_subqueue_completed_wake(). So it's supposed to be safe. > Reviewed-by: Jacob Keller <jacob.e.keller@intel.com> > >> - if (netif_queue_stopped(dev) && >> - rtl_tx_slots_avail(tp) >= R8169_TX_START_THRS) >> - netif_wake_queue(dev); >> + netif_subqueue_completed_wake(dev, 0, pkts_compl, bytes_compl, >> + rtl_tx_slots_avail(tp), >> + R8169_TX_START_THRS); >> /* >> * 8168 hack: TxPoll requests are lost when the Tx packets are >> * too close. Let's kick an extra TxPoll request when a burst
On 4/13/2023 12:36 PM, Heiner Kallweit wrote: > On 13.04.2023 21:25, Jacob Keller wrote: >> >> >> On 4/13/2023 12:16 PM, Heiner Kallweit wrote: >>> Use new net core macro netif_subqueue_completed_wake to simplify >>> the code of the tx cleanup path. >>> >>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> >>> --- >>> drivers/net/ethernet/realtek/r8169_main.c | 16 ++++------------ >>> 1 file changed, 4 insertions(+), 12 deletions(-) >>> >>> diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c >>> index 3f0b78fd9..5cfdb60ab 100644 >>> --- a/drivers/net/ethernet/realtek/r8169_main.c >>> +++ b/drivers/net/ethernet/realtek/r8169_main.c >>> @@ -4372,20 +4372,12 @@ static void rtl_tx(struct net_device *dev, struct rtl8169_private *tp, >>> } >>> >>> if (tp->dirty_tx != dirty_tx) { >>> - netdev_completed_queue(dev, pkts_compl, bytes_compl); >>> dev_sw_netstats_tx_add(dev, pkts_compl, bytes_compl); >>> + WRITE_ONCE(tp->dirty_tx, dirty_tx); >>> >>> - /* Sync with rtl8169_start_xmit: >>> - * - publish dirty_tx ring index (write barrier) >>> - * - refresh cur_tx ring index and queue status (read barrier) >>> - * May the current thread miss the stopped queue condition, >>> - * a racing xmit thread can only have a right view of the >>> - * ring status. >>> - */ >>> - smp_store_mb(tp->dirty_tx, dirty_tx); >> >> >> We used to use a smp_store_mb here, but now its safe to just WRITE_ONCE? >> Assuming that's correct: >> > netdev_completed_queue() has a smp_mb() and is called from > netif_subqueue_completed_wake(). So it's supposed to be safe. > Perfect, thanks for the explanation!
diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c index 3f0b78fd9..5cfdb60ab 100644 --- a/drivers/net/ethernet/realtek/r8169_main.c +++ b/drivers/net/ethernet/realtek/r8169_main.c @@ -4372,20 +4372,12 @@ static void rtl_tx(struct net_device *dev, struct rtl8169_private *tp, } if (tp->dirty_tx != dirty_tx) { - netdev_completed_queue(dev, pkts_compl, bytes_compl); dev_sw_netstats_tx_add(dev, pkts_compl, bytes_compl); + WRITE_ONCE(tp->dirty_tx, dirty_tx); - /* Sync with rtl8169_start_xmit: - * - publish dirty_tx ring index (write barrier) - * - refresh cur_tx ring index and queue status (read barrier) - * May the current thread miss the stopped queue condition, - * a racing xmit thread can only have a right view of the - * ring status. - */ - smp_store_mb(tp->dirty_tx, dirty_tx); - if (netif_queue_stopped(dev) && - rtl_tx_slots_avail(tp) >= R8169_TX_START_THRS) - netif_wake_queue(dev); + netif_subqueue_completed_wake(dev, 0, pkts_compl, bytes_compl, + rtl_tx_slots_avail(tp), + R8169_TX_START_THRS); /* * 8168 hack: TxPoll requests are lost when the Tx packets are * too close. Let's kick an extra TxPoll request when a burst
Use new net core macro netif_subqueue_completed_wake to simplify the code of the tx cleanup path. Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> --- drivers/net/ethernet/realtek/r8169_main.c | 16 ++++------------ 1 file changed, 4 insertions(+), 12 deletions(-)