Re: net: r8169: a question of memory barrier in the r8169 driver
Jia-Ju Bai : > > On 2018/1/19 9:11, Francois Romieu wrote: > > Jia-Ju Bai : > > [...] > > > The function rtl8169_start_xmit reads tp->dirty_tx in TX_FRAGS_READY_FOR: > > > if (unlikely(!TX_FRAGS_READY_FOR(tp, skb_shinfo(skb)->nr_frags))) { > > > netif_err(tp, drv, dev, "BUG! Tx Ring full when queue awake!\n"); > > > goto err_stop_0; > > > } > > > But there is no memory barrier around this code. > > > > > > Is there a possible data race here? > > This code would not even be needed if rtl8169_start_xmit was only your > > usual ndo_start_xmit handler: Realtek {ab / re}used it for GSO handling > > (see r8169_csum_workaround). > > > > If the test is not a no-op in this GSO context, it's racy. > > > > Thanks for reply. > I didn't clearly understand your meaning... It's fine. > I wonder whether there is a possible data race and whether a "smp_mb" is > needed before this code? > By the way, do you mean that this code can be removed? This code may be removed in a driver that properly stops itself its tx queueing in the ndo_start_xmit handler (I would still keep it as a bug detection helper but it's just a matter of taste). That's what the r8169 driver used to aim at. However, since e974604b453e87f8d864371786375d3d511fdf56, there is a piece of code where the r8169 driver iteratively uses its own ndo_start_xmit (without even checking its return value) in r8169_csum_workaround. It is racy. Now, let's forget races for a few seconds: how is r8169_csum_workaround supposed to work at all given that it does not care if (the "unlikely(...)" test in) rtl8169_start_xmit succeeds or not ? rtl8169_start_xmit can leave the skb as-is or map it to hardware descriptors (whence late release in rtl_tx). net/core/dev.c::dev_hard_start_xmit cares. r8169_csum_workaround doesn't. -- Ueimor
Re: net: r8169: a question of memory barrier in the r8169 driver
Peter Zijlstra : > On Fri, Jan 19, 2018 at 02:11:18AM +0100, Francois Romieu wrote: > > Peter Zijlstra : > > [...] > > > There is only 1 variable afaict. Memory barriers need at least 2 in > > > order to be able to do _anything_. > > > > I don't get your point: why don't {cur_tx, dirty_tx} qualify as said > > two variables ? > > There wasn't any cur_tx in the code you provided. /* A skbuff with nr_frags needs nr_frags+1 entries in the tx queue */ #define TX_FRAGS_READY_FOR(tp,nr_frags) \ (TX_SLOTS_AVAIL(tp) >= (nr_frags + 1)) #define TX_SLOTS_AVAIL(tp) \ (tp->dirty_tx + NUM_TX_DESC - tp->cur_tx) Both are also used in rtl_tx. I don't get your point. Even a single variable is scattered through the system. -- Ueimor
Re: net: r8169: a question of memory barrier in the r8169 driver
On Fri, Jan 19, 2018 at 02:11:18AM +0100, Francois Romieu wrote: > Peter Zijlstra : > [...] > > There is only 1 variable afaict. Memory barriers need at least 2 in > > order to be able to do _anything_. > > I don't get your point: why don't {cur_tx, dirty_tx} qualify as said > two variables ? There wasn't any cur_tx in the code you provided.
Re: net: r8169: a question of memory barrier in the r8169 driver
On 2018/1/19 9:11, Francois Romieu wrote: Jia-Ju Bai : [...] The function rtl8169_start_xmit reads tp->dirty_tx in TX_FRAGS_READY_FOR: if (unlikely(!TX_FRAGS_READY_FOR(tp, skb_shinfo(skb)->nr_frags))) { netif_err(tp, drv, dev, "BUG! Tx Ring full when queue awake!\n"); goto err_stop_0; } But there is no memory barrier around this code. Is there a possible data race here? This code would not even be needed if rtl8169_start_xmit was only your usual ndo_start_xmit handler: Realtek {ab / re}used it for GSO handling (see r8169_csum_workaround). If the test is not a no-op in this GSO context, it's racy. Thanks for reply. I didn't clearly understand your meaning... I wonder whether there is a possible data race and whether a "smp_mb" is needed before this code? By the way, do you mean that this code can be removed? Thanks, Jia-Ju Bai
Re: net: r8169: a question of memory barrier in the r8169 driver
Jia-Ju Bai : [...] > The function rtl8169_start_xmit reads tp->dirty_tx in TX_FRAGS_READY_FOR: > if (unlikely(!TX_FRAGS_READY_FOR(tp, skb_shinfo(skb)->nr_frags))) { > netif_err(tp, drv, dev, "BUG! Tx Ring full when queue awake!\n"); > goto err_stop_0; > } > But there is no memory barrier around this code. > > Is there a possible data race here? This code would not even be needed if rtl8169_start_xmit was only your usual ndo_start_xmit handler: Realtek {ab / re}used it for GSO handling (see r8169_csum_workaround). If the test is not a no-op in this GSO context, it's racy. -- Ueimor
Re: net: r8169: a question of memory barrier in the r8169 driver
Peter Zijlstra : [...] > There is only 1 variable afaict. Memory barriers need at least 2 in > order to be able to do _anything_. I don't get your point: why don't {cur_tx, dirty_tx} qualify as said two variables ? -- Ueimor
Re: net: r8169: a question of memory barrier in the r8169 driver
On Thu, Jan 18, 2018 at 10:06:17PM +0800, Jia-Ju Bai wrote: > In the rt8169 driver, the function "rtl_tx" uses "smp_mb" to sync the > writing operation with rtl8169_start_xmit: > if (tp->dirty_tx != dirty_tx) { > tp->dirty_tx = dirty_tx; > smp_mb(); > ... > } > The function rtl8169_start_xmit reads tp->dirty_tx in TX_FRAGS_READY_FOR: > if (unlikely(!TX_FRAGS_READY_FOR(tp, skb_shinfo(skb)->nr_frags))) { > netif_err(tp, drv, dev, "BUG! Tx Ring full when queue awake!\n"); > goto err_stop_0; > } > But there is no memory barrier around this code. > > Is there a possible data race here? > If not, how this data race is avoided? There is only 1 variable afaict. Memory barriers need at least 2 in order to be able to do _anything_.
net: r8169: a question of memory barrier in the r8169 driver
In the rt8169 driver, the function "rtl_tx" uses "smp_mb" to sync the writing operation with rtl8169_start_xmit: if (tp->dirty_tx != dirty_tx) { tp->dirty_tx = dirty_tx; smp_mb(); ... } The function rtl8169_start_xmit reads tp->dirty_tx in TX_FRAGS_READY_FOR: if (unlikely(!TX_FRAGS_READY_FOR(tp, skb_shinfo(skb)->nr_frags))) { netif_err(tp, drv, dev, "BUG! Tx Ring full when queue awake!\n"); goto err_stop_0; } But there is no memory barrier around this code. Is there a possible data race here? If not, how this data race is avoided? Thanks, Jia-Ju Bai