* [PATCH net-next v2 0/3] r8169: improve memory barriers
@ 2020-04-19 18:13 Heiner Kallweit
2020-04-19 18:14 ` [PATCH net-next v2 1/3] r8169: use smp_store_mb in rtl_tx Heiner Kallweit
` (3 more replies)
0 siblings, 4 replies; 10+ messages in thread
From: Heiner Kallweit @ 2020-04-19 18:13 UTC (permalink / raw)
To: Realtek linux nic maintainers, David Miller; +Cc: netdev
This series includes improvements for (too heavy) memory barriers.
Tested on x86-64 with RTL8168g chip version.
v2:
- drop patch 2 from the series
Heiner Kallweit (3):
r8169: use smp_store_mb in rtl_tx
r8169: replace dma_rmb with READ_ONCE in rtl_rx
r8169: use WRITE_ONCE instead of dma_wmb in rtl8169_mark_to_asic
drivers/net/ethernet/realtek/r8169_main.c | 24 ++++++++---------------
1 file changed, 8 insertions(+), 16 deletions(-)
--
2.26.1
^ permalink raw reply [flat|nested] 10+ messages in thread* [PATCH net-next v2 1/3] r8169: use smp_store_mb in rtl_tx 2020-04-19 18:13 [PATCH net-next v2 0/3] r8169: improve memory barriers Heiner Kallweit @ 2020-04-19 18:14 ` Heiner Kallweit 2020-04-19 18:15 ` [PATCH net-next v2 2/3] r8169: replace dma_rmb with READ_ONCE in rtl_rx Heiner Kallweit ` (2 subsequent siblings) 3 siblings, 0 replies; 10+ messages in thread From: Heiner Kallweit @ 2020-04-19 18:14 UTC (permalink / raw) To: Realtek linux nic maintainers, David Miller; +Cc: netdev Use helper smp_store_mb() instead of open-coding the functionality. Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> --- drivers/net/ethernet/realtek/r8169_main.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c index 1bc415d00..dd6113fd7 100644 --- a/drivers/net/ethernet/realtek/r8169_main.c +++ b/drivers/net/ethernet/realtek/r8169_main.c @@ -4438,7 +4438,6 @@ static void rtl_tx(struct net_device *dev, struct rtl8169_private *tp, tp->tx_stats.bytes += bytes_compl; u64_stats_update_end(&tp->tx_stats.syncp); - 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) @@ -4446,7 +4445,7 @@ static void rtl_tx(struct net_device *dev, struct rtl8169_private *tp, * a racing xmit thread can only have a right view of the * ring status. */ - smp_mb(); + smp_store_mb(tp->dirty_tx, dirty_tx); if (netif_queue_stopped(dev) && rtl_tx_slots_avail(tp, MAX_SKB_FRAGS)) { netif_wake_queue(dev); -- 2.26.1 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH net-next v2 2/3] r8169: replace dma_rmb with READ_ONCE in rtl_rx 2020-04-19 18:13 [PATCH net-next v2 0/3] r8169: improve memory barriers Heiner Kallweit 2020-04-19 18:14 ` [PATCH net-next v2 1/3] r8169: use smp_store_mb in rtl_tx Heiner Kallweit @ 2020-04-19 18:15 ` Heiner Kallweit 2020-04-19 18:16 ` [PATCH net-next v2 3/3] r8169: use WRITE_ONCE instead of dma_wmb in rtl8169_mark_to_asic Heiner Kallweit 2020-04-19 21:54 ` [PATCH net-next v2 0/3] r8169: improve memory barriers Heiner Kallweit 3 siblings, 0 replies; 10+ messages in thread From: Heiner Kallweit @ 2020-04-19 18:15 UTC (permalink / raw) To: Realtek linux nic maintainers, David Miller; +Cc: netdev We want to ensure that desc->opts1 is read before desc->opts2. This doesn't require a full compiler barrier. READ_ONCE provides the ordering guarantee we need. Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> --- drivers/net/ethernet/realtek/r8169_main.c | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-) diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c index dd6113fd7..2fc65aca3 100644 --- a/drivers/net/ethernet/realtek/r8169_main.c +++ b/drivers/net/ethernet/realtek/r8169_main.c @@ -1548,7 +1548,7 @@ static inline u32 rtl8169_tx_vlan_tag(struct sk_buff *skb) static void rtl8169_rx_vlan_tag(struct RxDesc *desc, struct sk_buff *skb) { - u32 opts2 = le32_to_cpu(desc->opts2); + u32 opts2 = le32_to_cpu(READ_ONCE(desc->opts2)); if (opts2 & RxVlanTag) __vlan_hwaccel_put_tag(skb, htons(ETH_P_8021Q), swab16(opts2 & 0xffff)); @@ -4490,16 +4490,11 @@ static int rtl_rx(struct net_device *dev, struct rtl8169_private *tp, u32 budget struct RxDesc *desc = tp->RxDescArray + entry; u32 status; - status = le32_to_cpu(desc->opts1); + /* Use READ_ONCE to order descriptor field reads */ + status = le32_to_cpu(READ_ONCE(desc->opts1)); if (status & DescOwn) break; - /* This barrier is needed to keep us from reading - * any other fields out of the Rx descriptor until - * we know the status of DescOwn - */ - dma_rmb(); - if (unlikely(status & RxRES)) { netif_info(tp, rx_err, dev, "Rx ERROR. status = %08x\n", status); -- 2.26.1 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH net-next v2 3/3] r8169: use WRITE_ONCE instead of dma_wmb in rtl8169_mark_to_asic 2020-04-19 18:13 [PATCH net-next v2 0/3] r8169: improve memory barriers Heiner Kallweit 2020-04-19 18:14 ` [PATCH net-next v2 1/3] r8169: use smp_store_mb in rtl_tx Heiner Kallweit 2020-04-19 18:15 ` [PATCH net-next v2 2/3] r8169: replace dma_rmb with READ_ONCE in rtl_rx Heiner Kallweit @ 2020-04-19 18:16 ` Heiner Kallweit 2020-04-19 19:00 ` Petko Manolov 2020-04-19 21:54 ` [PATCH net-next v2 0/3] r8169: improve memory barriers Heiner Kallweit 3 siblings, 1 reply; 10+ messages in thread From: Heiner Kallweit @ 2020-04-19 18:16 UTC (permalink / raw) To: Realtek linux nic maintainers, David Miller; +Cc: netdev We want to ensure that desc->opts1 is written as last descriptor field. This doesn't require a full compiler barrier, WRITE_ONCE provides the ordering guarantee we need. Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> --- drivers/net/ethernet/realtek/r8169_main.c | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c index 2fc65aca3..3e4ed2528 100644 --- a/drivers/net/ethernet/realtek/r8169_main.c +++ b/drivers/net/ethernet/realtek/r8169_main.c @@ -3892,11 +3892,9 @@ static inline void rtl8169_mark_to_asic(struct RxDesc *desc) { u32 eor = le32_to_cpu(desc->opts1) & RingEnd; - desc->opts2 = 0; - /* Force memory writes to complete before releasing descriptor */ - dma_wmb(); - - desc->opts1 = cpu_to_le32(DescOwn | eor | R8169_RX_BUF_SIZE); + /* Ensure ordering of writes */ + WRITE_ONCE(desc->opts2, 0); + WRITE_ONCE(desc->opts1, cpu_to_le32(DescOwn | eor | R8169_RX_BUF_SIZE)); } static struct page *rtl8169_alloc_rx_data(struct rtl8169_private *tp, @@ -3919,7 +3917,7 @@ static struct page *rtl8169_alloc_rx_data(struct rtl8169_private *tp, return NULL; } - desc->addr = cpu_to_le64(mapping); + WRITE_ONCE(desc->addr, cpu_to_le64(mapping)); rtl8169_mark_to_asic(desc); return data; -- 2.26.1 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH net-next v2 3/3] r8169: use WRITE_ONCE instead of dma_wmb in rtl8169_mark_to_asic 2020-04-19 18:16 ` [PATCH net-next v2 3/3] r8169: use WRITE_ONCE instead of dma_wmb in rtl8169_mark_to_asic Heiner Kallweit @ 2020-04-19 19:00 ` Petko Manolov 2020-04-19 21:03 ` Heiner Kallweit 0 siblings, 1 reply; 10+ messages in thread From: Petko Manolov @ 2020-04-19 19:00 UTC (permalink / raw) To: Heiner Kallweit; +Cc: Realtek linux nic maintainers, David Miller, netdev On 20-04-19 20:16:21, Heiner Kallweit wrote: > We want to ensure that desc->opts1 is written as last descriptor field. > This doesn't require a full compiler barrier, WRITE_ONCE provides the > ordering guarantee we need. > > Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> > --- > drivers/net/ethernet/realtek/r8169_main.c | 10 ++++------ > 1 file changed, 4 insertions(+), 6 deletions(-) > > diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c > index 2fc65aca3..3e4ed2528 100644 > --- a/drivers/net/ethernet/realtek/r8169_main.c > +++ b/drivers/net/ethernet/realtek/r8169_main.c > @@ -3892,11 +3892,9 @@ static inline void rtl8169_mark_to_asic(struct RxDesc *desc) > { > u32 eor = le32_to_cpu(desc->opts1) & RingEnd; > > - desc->opts2 = 0; > - /* Force memory writes to complete before releasing descriptor */ > - dma_wmb(); If dma_wmb() was really ever needed here you should leave it even after you order these writes with WRITE_ONCE(). If not, then good riddance. Just saying, i am not familiar with the hardware nor with the driver. :) Petko > - > - desc->opts1 = cpu_to_le32(DescOwn | eor | R8169_RX_BUF_SIZE); > + /* Ensure ordering of writes */ > + WRITE_ONCE(desc->opts2, 0); > + WRITE_ONCE(desc->opts1, cpu_to_le32(DescOwn | eor | R8169_RX_BUF_SIZE)); > } > > static struct page *rtl8169_alloc_rx_data(struct rtl8169_private *tp, > @@ -3919,7 +3917,7 @@ static struct page *rtl8169_alloc_rx_data(struct rtl8169_private *tp, > return NULL; > } > > - desc->addr = cpu_to_le64(mapping); > + WRITE_ONCE(desc->addr, cpu_to_le64(mapping)); > rtl8169_mark_to_asic(desc); > > return data; > -- > 2.26.1 > > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH net-next v2 3/3] r8169: use WRITE_ONCE instead of dma_wmb in rtl8169_mark_to_asic 2020-04-19 19:00 ` Petko Manolov @ 2020-04-19 21:03 ` Heiner Kallweit 2020-04-19 21:38 ` Petko Manolov 2020-04-19 22:33 ` Eric Dumazet 0 siblings, 2 replies; 10+ messages in thread From: Heiner Kallweit @ 2020-04-19 21:03 UTC (permalink / raw) To: Realtek linux nic maintainers, David Miller, netdev On 19.04.2020 21:00, Petko Manolov wrote: > On 20-04-19 20:16:21, Heiner Kallweit wrote: >> We want to ensure that desc->opts1 is written as last descriptor field. >> This doesn't require a full compiler barrier, WRITE_ONCE provides the >> ordering guarantee we need. >> >> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> >> --- >> drivers/net/ethernet/realtek/r8169_main.c | 10 ++++------ >> 1 file changed, 4 insertions(+), 6 deletions(-) >> >> diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c >> index 2fc65aca3..3e4ed2528 100644 >> --- a/drivers/net/ethernet/realtek/r8169_main.c >> +++ b/drivers/net/ethernet/realtek/r8169_main.c >> @@ -3892,11 +3892,9 @@ static inline void rtl8169_mark_to_asic(struct RxDesc *desc) >> { >> u32 eor = le32_to_cpu(desc->opts1) & RingEnd; >> >> - desc->opts2 = 0; >> - /* Force memory writes to complete before releasing descriptor */ >> - dma_wmb(); > > If dma_wmb() was really ever needed here you should leave it even after you > order these writes with WRITE_ONCE(). If not, then good riddance. > My understanding is that we have to avoid transferring ownership of descriptor to device (by setting DescOwn bit) before opts2 field has been written. Using WRITE_ONCE() should be sufficient to prevent the compiler from merging or reordering the writes. At least that's how I read the process_level() example in https://www.kernel.org/doc/Documentation/memory-barriers.txt > Just saying, i am not familiar with the hardware nor with the driver. :) > > > Petko > > >> - >> - desc->opts1 = cpu_to_le32(DescOwn | eor | R8169_RX_BUF_SIZE); >> + /* Ensure ordering of writes */ >> + WRITE_ONCE(desc->opts2, 0); >> + WRITE_ONCE(desc->opts1, cpu_to_le32(DescOwn | eor | R8169_RX_BUF_SIZE)); >> } >> >> static struct page *rtl8169_alloc_rx_data(struct rtl8169_private *tp, >> @@ -3919,7 +3917,7 @@ static struct page *rtl8169_alloc_rx_data(struct rtl8169_private *tp, >> return NULL; >> } >> >> - desc->addr = cpu_to_le64(mapping); >> + WRITE_ONCE(desc->addr, cpu_to_le64(mapping)); >> rtl8169_mark_to_asic(desc); >> >> return data; >> -- >> 2.26.1 >> >> ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH net-next v2 3/3] r8169: use WRITE_ONCE instead of dma_wmb in rtl8169_mark_to_asic 2020-04-19 21:03 ` Heiner Kallweit @ 2020-04-19 21:38 ` Petko Manolov 2020-04-19 21:52 ` Heiner Kallweit 2020-04-19 22:33 ` Eric Dumazet 1 sibling, 1 reply; 10+ messages in thread From: Petko Manolov @ 2020-04-19 21:38 UTC (permalink / raw) To: Heiner Kallweit; +Cc: Realtek linux nic maintainers, David Miller, netdev On 20-04-19 23:03:57, Heiner Kallweit wrote: > On 19.04.2020 21:00, Petko Manolov wrote: > > On 20-04-19 20:16:21, Heiner Kallweit wrote: > >> We want to ensure that desc->opts1 is written as last descriptor field. > >> This doesn't require a full compiler barrier, WRITE_ONCE provides the > >> ordering guarantee we need. > >> > >> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> > >> --- > >> drivers/net/ethernet/realtek/r8169_main.c | 10 ++++------ > >> 1 file changed, 4 insertions(+), 6 deletions(-) > >> > >> diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c > >> index 2fc65aca3..3e4ed2528 100644 > >> --- a/drivers/net/ethernet/realtek/r8169_main.c > >> +++ b/drivers/net/ethernet/realtek/r8169_main.c > >> @@ -3892,11 +3892,9 @@ static inline void rtl8169_mark_to_asic(struct RxDesc *desc) > >> { > >> u32 eor = le32_to_cpu(desc->opts1) & RingEnd; > >> > >> - desc->opts2 = 0; > >> - /* Force memory writes to complete before releasing descriptor */ > >> - dma_wmb(); > > > > If dma_wmb() was really ever needed here you should leave it even after you > > order these writes with WRITE_ONCE(). If not, then good riddance. > > > My understanding is that we have to avoid transferring ownership of > descriptor to device (by setting DescOwn bit) before opts2 field > has been written. Using WRITE_ONCE() should be sufficient to prevent > the compiler from merging or reordering the writes. > At least that's how I read the process_level() example in > https://www.kernel.org/doc/Documentation/memory-barriers.txt WRITE_ONCE() will prevent the compiler from reordering, but not the CPU. On x86 this code will run just fine, but not on ARM or PPC. Based on your description above i think dma_wmb() is still needed. Petko > > Just saying, i am not familiar with the hardware nor with the driver. :) > > > > > > Petko > > > > > >> - > >> - desc->opts1 = cpu_to_le32(DescOwn | eor | R8169_RX_BUF_SIZE); > >> + /* Ensure ordering of writes */ > >> + WRITE_ONCE(desc->opts2, 0); > >> + WRITE_ONCE(desc->opts1, cpu_to_le32(DescOwn | eor | R8169_RX_BUF_SIZE)); > >> } > >> > >> static struct page *rtl8169_alloc_rx_data(struct rtl8169_private *tp, > >> @@ -3919,7 +3917,7 @@ static struct page *rtl8169_alloc_rx_data(struct rtl8169_private *tp, > >> return NULL; > >> } > >> > >> - desc->addr = cpu_to_le64(mapping); > >> + WRITE_ONCE(desc->addr, cpu_to_le64(mapping)); > >> rtl8169_mark_to_asic(desc); > >> > >> return data; > >> -- > >> 2.26.1 > >> > >> > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH net-next v2 3/3] r8169: use WRITE_ONCE instead of dma_wmb in rtl8169_mark_to_asic 2020-04-19 21:38 ` Petko Manolov @ 2020-04-19 21:52 ` Heiner Kallweit 0 siblings, 0 replies; 10+ messages in thread From: Heiner Kallweit @ 2020-04-19 21:52 UTC (permalink / raw) To: Realtek linux nic maintainers, David Miller, netdev On 19.04.2020 23:38, Petko Manolov wrote: > On 20-04-19 23:03:57, Heiner Kallweit wrote: >> On 19.04.2020 21:00, Petko Manolov wrote: >>> On 20-04-19 20:16:21, Heiner Kallweit wrote: >>>> We want to ensure that desc->opts1 is written as last descriptor field. >>>> This doesn't require a full compiler barrier, WRITE_ONCE provides the >>>> ordering guarantee we need. >>>> >>>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> >>>> --- >>>> drivers/net/ethernet/realtek/r8169_main.c | 10 ++++------ >>>> 1 file changed, 4 insertions(+), 6 deletions(-) >>>> >>>> diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c >>>> index 2fc65aca3..3e4ed2528 100644 >>>> --- a/drivers/net/ethernet/realtek/r8169_main.c >>>> +++ b/drivers/net/ethernet/realtek/r8169_main.c >>>> @@ -3892,11 +3892,9 @@ static inline void rtl8169_mark_to_asic(struct RxDesc *desc) >>>> { >>>> u32 eor = le32_to_cpu(desc->opts1) & RingEnd; >>>> >>>> - desc->opts2 = 0; >>>> - /* Force memory writes to complete before releasing descriptor */ >>>> - dma_wmb(); >>> >>> If dma_wmb() was really ever needed here you should leave it even after you >>> order these writes with WRITE_ONCE(). If not, then good riddance. >>> >> My understanding is that we have to avoid transferring ownership of >> descriptor to device (by setting DescOwn bit) before opts2 field >> has been written. Using WRITE_ONCE() should be sufficient to prevent >> the compiler from merging or reordering the writes. >> At least that's how I read the process_level() example in >> https://www.kernel.org/doc/Documentation/memory-barriers.txt > > WRITE_ONCE() will prevent the compiler from reordering, but not the CPU. On x86 > this code will run just fine, but not on ARM or PPC. Based on your description > above i think dma_wmb() is still needed. > After reading a little about ARMv8-A and weakly-ordered memory I tend to agree with you. It's a pity that I don't know of any use of Realtek network chips on such a weakly-ordered platform (what of course doesn't mean it doesn't exist). This would really help me in testing and avoid such "but it works on x86" .. > > Petko > > >>> Just saying, i am not familiar with the hardware nor with the driver. :) >>> >>> >>> Petko >>> >>> >>>> - >>>> - desc->opts1 = cpu_to_le32(DescOwn | eor | R8169_RX_BUF_SIZE); >>>> + /* Ensure ordering of writes */ >>>> + WRITE_ONCE(desc->opts2, 0); >>>> + WRITE_ONCE(desc->opts1, cpu_to_le32(DescOwn | eor | R8169_RX_BUF_SIZE)); >>>> } >>>> >>>> static struct page *rtl8169_alloc_rx_data(struct rtl8169_private *tp, >>>> @@ -3919,7 +3917,7 @@ static struct page *rtl8169_alloc_rx_data(struct rtl8169_private *tp, >>>> return NULL; >>>> } >>>> >>>> - desc->addr = cpu_to_le64(mapping); >>>> + WRITE_ONCE(desc->addr, cpu_to_le64(mapping)); >>>> rtl8169_mark_to_asic(desc); >>>> >>>> return data; >>>> -- >>>> 2.26.1 >>>> >>>> >> ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH net-next v2 3/3] r8169: use WRITE_ONCE instead of dma_wmb in rtl8169_mark_to_asic 2020-04-19 21:03 ` Heiner Kallweit 2020-04-19 21:38 ` Petko Manolov @ 2020-04-19 22:33 ` Eric Dumazet 1 sibling, 0 replies; 10+ messages in thread From: Eric Dumazet @ 2020-04-19 22:33 UTC (permalink / raw) To: Heiner Kallweit, Realtek linux nic maintainers, David Miller, netdev On 4/19/20 2:03 PM, Heiner Kallweit wrote: > On 19.04.2020 21:00, Petko Manolov wrote: >> On 20-04-19 20:16:21, Heiner Kallweit wrote: >>> We want to ensure that desc->opts1 is written as last descriptor field. >>> This doesn't require a full compiler barrier, WRITE_ONCE provides the >>> ordering guarantee we need. >>> >>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> >>> --- >>> drivers/net/ethernet/realtek/r8169_main.c | 10 ++++------ >>> 1 file changed, 4 insertions(+), 6 deletions(-) >>> >>> diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c >>> index 2fc65aca3..3e4ed2528 100644 >>> --- a/drivers/net/ethernet/realtek/r8169_main.c >>> +++ b/drivers/net/ethernet/realtek/r8169_main.c >>> @@ -3892,11 +3892,9 @@ static inline void rtl8169_mark_to_asic(struct RxDesc *desc) >>> { >>> u32 eor = le32_to_cpu(desc->opts1) & RingEnd; >>> >>> - desc->opts2 = 0; >>> - /* Force memory writes to complete before releasing descriptor */ >>> - dma_wmb(); >> >> If dma_wmb() was really ever needed here you should leave it even after you >> order these writes with WRITE_ONCE(). If not, then good riddance. >> > My understanding is that we have to avoid transferring ownership of > descriptor to device (by setting DescOwn bit) before opts2 field > has been written. Using WRITE_ONCE() should be sufficient to prevent > the compiler from merging or reordering the writes. > At least that's how I read the process_level() example in > https://www.kernel.org/doc/Documentation/memory-barriers.txt > >> Just saying, i am not familiar with the hardware nor with the driver. :) >> >> >> Petko >> >> >>> - >>> - desc->opts1 = cpu_to_le32(DescOwn | eor | R8169_RX_BUF_SIZE); >>> + /* Ensure ordering of writes */ >>> + WRITE_ONCE(desc->opts2, 0); >>> + WRITE_ONCE(desc->opts1, cpu_to_le32(DescOwn | eor | R8169_RX_BUF_SIZE)); No, this is not correct. WRITE_ONCE(X, ...) WRITE_ONCE(Y, ...) has no guarantee X is written before Y Sure, the compiler is making sure to perform one write for X, one for Y, but you want stronger semantic than that. You want to keep dma_wmb() However, this part of your patch seems fine : - desc->opts1 = cpu_to_le32(DescOwn | eor | R8169_RX_BUF_SIZE); + WRITE_ONCE(desc->opts1, cpu_to_le32(DescOwn | eor | R8169_RX_BUF_SIZE)); >>> } >>> >>> static struct page *rtl8169_alloc_rx_data(struct rtl8169_private *tp, >>> @@ -3919,7 +3917,7 @@ static struct page *rtl8169_alloc_rx_data(struct rtl8169_private *tp, >>> return NULL; >>> } >>> >>> - desc->addr = cpu_to_le64(mapping); >>> + WRITE_ONCE(desc->addr, cpu_to_le64(mapping)); >>> rtl8169_mark_to_asic(desc); >>> >>> return data; >>> -- >>> 2.26.1 >>> >>> > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH net-next v2 0/3] r8169: improve memory barriers 2020-04-19 18:13 [PATCH net-next v2 0/3] r8169: improve memory barriers Heiner Kallweit ` (2 preceding siblings ...) 2020-04-19 18:16 ` [PATCH net-next v2 3/3] r8169: use WRITE_ONCE instead of dma_wmb in rtl8169_mark_to_asic Heiner Kallweit @ 2020-04-19 21:54 ` Heiner Kallweit 3 siblings, 0 replies; 10+ messages in thread From: Heiner Kallweit @ 2020-04-19 21:54 UTC (permalink / raw) To: Realtek linux nic maintainers, David Miller; +Cc: netdev On 19.04.2020 20:13, Heiner Kallweit wrote: > This series includes improvements for (too heavy) memory barriers. > Tested on x86-64 with RTL8168g chip version. > A recent conversation convinced me that few of these change are not safe on platforms with weakly-ordered memory. Therefore please disregard the series. > v2: > - drop patch 2 from the series > > Heiner Kallweit (3): > r8169: use smp_store_mb in rtl_tx > r8169: replace dma_rmb with READ_ONCE in rtl_rx > r8169: use WRITE_ONCE instead of dma_wmb in rtl8169_mark_to_asic > > drivers/net/ethernet/realtek/r8169_main.c | 24 ++++++++--------------- > 1 file changed, 8 insertions(+), 16 deletions(-) > ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2020-04-19 22:33 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-04-19 18:13 [PATCH net-next v2 0/3] r8169: improve memory barriers Heiner Kallweit 2020-04-19 18:14 ` [PATCH net-next v2 1/3] r8169: use smp_store_mb in rtl_tx Heiner Kallweit 2020-04-19 18:15 ` [PATCH net-next v2 2/3] r8169: replace dma_rmb with READ_ONCE in rtl_rx Heiner Kallweit 2020-04-19 18:16 ` [PATCH net-next v2 3/3] r8169: use WRITE_ONCE instead of dma_wmb in rtl8169_mark_to_asic Heiner Kallweit 2020-04-19 19:00 ` Petko Manolov 2020-04-19 21:03 ` Heiner Kallweit 2020-04-19 21:38 ` Petko Manolov 2020-04-19 21:52 ` Heiner Kallweit 2020-04-19 22:33 ` Eric Dumazet 2020-04-19 21:54 ` [PATCH net-next v2 0/3] r8169: improve memory barriers Heiner Kallweit
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.