[PATCH net-next] r8152: divide the tx and rx bottom functions
Move the tx bottom function from NAPI to a new tasklet. Then, for multi-cores, the bottom functions of tx and rx may be run at same time with different cores. This is used to improve performance. Signed-off-by: Hayes Wang --- drivers/net/usb/r8152.c | 39 ++- 1 file changed, 26 insertions(+), 13 deletions(-) diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c index 40d18e866269..3ed9f8e082c9 100644 --- a/drivers/net/usb/r8152.c +++ b/drivers/net/usb/r8152.c @@ -619,7 +619,7 @@ enum rtl8152_flags { RTL8152_LINK_CHG, SELECTIVE_SUSPEND, PHY_RESET, - SCHEDULE_NAPI, + SCHEDULE_TASKLET, GREEN_ETHERNET, DELL_TB_RX_AGG_BUG, }; @@ -733,6 +733,7 @@ struct r8152 { #ifdef CONFIG_PM_SLEEP struct notifier_block pm_notifier; #endif + struct tasklet_struct tx_tl; struct rtl_ops { void (*init)(struct r8152 *); @@ -1401,7 +1402,7 @@ static void write_bulk_callback(struct urb *urb) return; if (!skb_queue_empty(&tp->tx_queue)) - napi_schedule(&tp->napi); + tasklet_schedule(&tp->tx_tl); } static void intr_callback(struct urb *urb) @@ -2179,8 +2180,12 @@ static void tx_bottom(struct r8152 *tp) } while (res == 0); } -static void bottom_half(struct r8152 *tp) +static void bottom_half(unsigned long data) { + struct r8152 *tp; + + tp = (struct r8152 *)data; + if (test_bit(RTL8152_UNPLUG, &tp->flags)) return; @@ -2192,7 +2197,7 @@ static void bottom_half(struct r8152 *tp) if (!netif_carrier_ok(tp->netdev)) return; - clear_bit(SCHEDULE_NAPI, &tp->flags); + clear_bit(SCHEDULE_TASKLET, &tp->flags); tx_bottom(tp); } @@ -2203,16 +2208,12 @@ static int r8152_poll(struct napi_struct *napi, int budget) int work_done; work_done = rx_bottom(tp, budget); - bottom_half(tp); if (work_done < budget) { if (!napi_complete_done(napi, work_done)) goto out; if (!list_empty(&tp->rx_done)) napi_schedule(napi); - else if (!skb_queue_empty(&tp->tx_queue) && -!list_empty(&tp->tx_free)) - napi_schedule(napi); } out: @@ -2366,11 +2367,11 @@ static netdev_tx_t rtl8152_start_xmit(struct sk_buff *skb, if (!list_empty(&tp->tx_free)) { if (test_bit(SELECTIVE_SUSPEND, &tp->flags)) { - set_bit(SCHEDULE_NAPI, &tp->flags); + set_bit(SCHEDULE_TASKLET, &tp->flags); schedule_delayed_work(&tp->schedule, 0); } else { usb_mark_last_busy(tp->udev); - napi_schedule(&tp->napi); + tasklet_schedule(&tp->tx_tl); } } else if (skb_queue_len(&tp->tx_queue) > tp->tx_qlen) { netif_stop_queue(netdev); @@ -4020,9 +4021,11 @@ static void set_carrier(struct r8152 *tp) } else { if (netif_carrier_ok(netdev)) { netif_carrier_off(netdev); + tasklet_disable(&tp->tx_tl); napi_disable(napi); tp->rtl_ops.disable(tp); napi_enable(napi); + tasklet_enable(&tp->tx_tl); netif_info(tp, link, netdev, "carrier off\n"); } } @@ -4055,10 +4058,10 @@ static void rtl_work_func_t(struct work_struct *work) if (test_and_clear_bit(RTL8152_SET_RX_MODE, &tp->flags)) _rtl8152_set_rx_mode(tp->netdev); - /* don't schedule napi before linking */ - if (test_and_clear_bit(SCHEDULE_NAPI, &tp->flags) && + /* don't schedule tasket before linking */ + if (test_and_clear_bit(SCHEDULE_TASKLET, &tp->flags) && netif_carrier_ok(tp->netdev)) - napi_schedule(&tp->napi); + tasklet_schedule(&tp->tx_tl); mutex_unlock(&tp->control); @@ -4144,6 +4147,7 @@ static int rtl8152_open(struct net_device *netdev) goto out_unlock; } napi_enable(&tp->napi); + tasklet_enable(&tp->tx_tl); mutex_unlock(&tp->control); @@ -4171,6 +4175,7 @@ static int rtl8152_close(struct net_device *netdev) #ifdef CONFIG_PM_SLEEP unregister_pm_notifier(&tp->pm_notifier); #endif + tasklet_disable(&tp->tx_tl); if (!test_bit(RTL8152_UNPLUG, &tp->flags)) napi_disable(&tp->napi); clear_bit(WORK_ENABLE, &tp->flags); @@ -4440,6 +4445,7 @@ static int rtl8152_pre_reset(struct usb_interface *intf) return 0; netif_stop_queue(netdev); + tasklet_disable(&tp->tx_tl); napi_disable(&tp->napi); clear_bit(WORK_ENABLE, &tp->flags);
Re: [PATCH net-next] r8152: divide the tx and rx bottom functions
From: Hayes Wang Date: Wed, 14 Aug 2019 16:30:17 +0800 > Move the tx bottom function from NAPI to a new tasklet. Then, for > multi-cores, the bottom functions of tx and rx may be run at same > time with different cores. This is used to improve performance. > > Signed-off-by: Hayes Wang Theoretically, yes. But do you have actual performance numbers showing this to be worth the change? Always provide performance numbers with changes that are supposed to improve performance.
RE: [PATCH net-next] r8152: divide the tx and rx bottom functions
David Miller [mailto:da...@davemloft.net] > Sent: Friday, August 16, 2019 4:59 AM [...] > Theoretically, yes. > > But do you have actual performance numbers showing this to be worth > the change? > > Always provide performance numbers with changes that are supposed to > improve performance. On x86, they are almost the same. Tx/Rx: 943/943 Mbits/sec -> 945/944 For arm platform, Tx/Rx: 917/917 Mbits/sec -> 933/933 Improve about 1.74%. Best Regards, Hayes
Re: [PATCH net-next] r8152: divide the tx and rx bottom functions
From: Hayes Wang Date: Fri, 16 Aug 2019 02:59:16 + > David Miller [mailto:da...@davemloft.net] >> Sent: Friday, August 16, 2019 4:59 AM > [...] >> Theoretically, yes. >> >> But do you have actual performance numbers showing this to be worth >> the change? >> >> Always provide performance numbers with changes that are supposed to >> improve performance. > > On x86, they are almost the same. > Tx/Rx: 943/943 Mbits/sec -> 945/944 > > For arm platform, > Tx/Rx: 917/917 Mbits/sec -> 933/933 > Improve about 1.74%. Belongs in the commit message.
Re: [PATCH net-next] r8152: divide the tx and rx bottom functions
On 8/14/19 10:30 AM, Hayes Wang wrote: > Move the tx bottom function from NAPI to a new tasklet. Then, for > multi-cores, the bottom functions of tx and rx may be run at same > time with different cores. This is used to improve performance. > > tasklet and NAPI are scheduled on the same core (the current cpu calling napi_schedule() or tasklet_schedule()) I would rather not add this dubious tasklet, and instead try to understand what is wrong in this driver ;) The various napi_schedule() calls are suspect IMO. Also rtl8152_start_xmit() uses skb_queue_tail(&tp->tx_queue, skb); But I see nothing really kicking the transmit if tx_free is empty ? tx_bottom() is only called from bottom_half() (called from r8152_poll())
RE: [PATCH net-next] r8152: divide the tx and rx bottom functions
Eric Dumazet [mailto:eric.duma...@gmail.com] > Sent: Friday, August 16, 2019 2:40 PM [...] > tasklet and NAPI are scheduled on the same core (the current > cpu calling napi_schedule() or tasklet_schedule()) > > I would rather not add this dubious tasklet, and instead try to understand > what is wrong in this driver ;) > > The various napi_schedule() calls are suspect IMO. The original method as following. static int r8152_poll(struct napi_struct *napi, int budget) { struct r8152 *tp = container_of(napi, struct r8152, napi); int work_done; work_done = rx_bottom(tp, budget); <-- RX bottom_half(tp); <-- Tx (tx_bottom) [...] The rx_bottom and tx_bottom would only be called in r8152_poll. That is, tx_bottom wouldn't be run unless rx_bottom is finished. And, rx_bottom would be called if tx_bottom is running. If the traffic is busy. rx_bottom or tx_bottom may take a lot of time to deal with the packets. And the one would increase the latency time for the other one. Therefore, when I separate the tx_bottom and rx_bottom to different tasklet and napi, the callback functions of tx and rx may schedule the tasklet and napi to different cpu. Then, the rx_bottom and tx_bottom may be run at the same time. Take our arm platform for example. There are five cpus to handle the interrupt of USB host controller. When the rx is completed, cpu #1 may handle the interrupt and napi would be scheduled. When the tx is finished, cpu #2 may handle the interrupt and the tasklet is scheduled. Then, napi is run on cpu #1, and tasklet is run on cpu #2. > Also rtl8152_start_xmit() uses skb_queue_tail(&tp->tx_queue, skb); > > But I see nothing really kicking the transmit if tx_free is empty ? Tx callback function "write_bulk_callback" would deal with it. The callback function would check if there are packets waiting to be sent. Best Regards, Hayes
Re: [PATCH net-next] r8152: divide the tx and rx bottom functions
On 8/16/19 10:10 AM, Hayes Wang wrote: > Eric Dumazet [mailto:eric.duma...@gmail.com] >> Sent: Friday, August 16, 2019 2:40 PM > [...] >> tasklet and NAPI are scheduled on the same core (the current >> cpu calling napi_schedule() or tasklet_schedule()) >> >> I would rather not add this dubious tasklet, and instead try to understand >> what is wrong in this driver ;) >> >> The various napi_schedule() calls are suspect IMO. > > The original method as following. > > static int r8152_poll(struct napi_struct *napi, int budget) > { > struct r8152 *tp = container_of(napi, struct r8152, napi); > int work_done; > > work_done = rx_bottom(tp, budget); <-- RX > bottom_half(tp); <-- Tx (tx_bottom) > [...] > > The rx_bottom and tx_bottom would only be called in r8152_poll. > That is, tx_bottom wouldn't be run unless rx_bottom is finished. > And, rx_bottom would be called if tx_bottom is running. > > If the traffic is busy. rx_bottom or tx_bottom may take a lot > of time to deal with the packets. And the one would increase > the latency time for the other one. > > Therefore, when I separate the tx_bottom and rx_bottom to > different tasklet and napi, the callback functions of tx and > rx may schedule the tasklet and napi to different cpu. Then, > the rx_bottom and tx_bottom may be run at the same time. Your patch makes absolutely no guarantee of doing what you want, I am afraid. > > Take our arm platform for example. There are five cpus to > handle the interrupt of USB host controller. When the rx is > completed, cpu #1 may handle the interrupt and napi would > be scheduled. When the tx is finished, cpu #2 may handle > the interrupt and the tasklet is scheduled. Then, napi is > run on cpu #1, and tasklet is run on cpu #2. > >> Also rtl8152_start_xmit() uses skb_queue_tail(&tp->tx_queue, skb); >> >> But I see nothing really kicking the transmit if tx_free is empty ? > > Tx callback function "write_bulk_callback" would deal with it. > The callback function would check if there are packets waiting > to be sent. Which callback ? After an idle period (no activity, no prior packets being tx-completed ...), a packet is sent by the upper stack, enters the ndo_start_xmit() of a network driver. This driver ndo_start_xmit() simply adds an skb to a local list, and returns. Where/how is scheduled this callback ? Some kind of timer ? An (unrelated) incoming packet ? > > > Best Regards, > Hayes > >
RE: [PATCH net-next] r8152: divide the tx and rx bottom functions
Eric Dumazet [mailto:eric.duma...@gmail.com] > Sent: Friday, August 16, 2019 4:20 PM [...] > Which callback ? The USB device has two endpoints for Tx and Rx. If I submit tx or rx URB to the USB host controller, the relative callback functions would be called, when they are finished. For rx, it is read_bulk_callback. For tx, it is write_bulk_callback. > After an idle period (no activity, no prior packets being tx-completed ...), > a packet is sent by the upper stack, enters the ndo_start_xmit() of a network > driver. > > This driver ndo_start_xmit() simply adds an skb to a local list, and returns. Base on the current method (without tasklet), when ndo_start_xmit() is called, napi_schedule is called only if there is at least one free buffer (!list_empty(&tp->tx_free)) to transmit the packet. Then, the flow would be as following. - Call r8152_poll -- Call bottom_half --- Call tx_bottom Call r8152_tx_agg_fill - submit tx urb - Call write_bulk_callback if tx is completed When the tx transfer is completed, write_bulk_callback would be called. And it would check if there is any tx packet in &tp->tx_queue and determine whether it is necessary to schedule the napi again or not. > Where/how is scheduled this callback ? For tx, you could find the following code in r8152_tx_agg_fill(). usb_fill_bulk_urb(agg->urb, tp->udev, usb_sndbulkpipe(tp->udev, 2), agg->head, (int)(tx_data - (u8 *)agg->head), (usb_complete_t)write_bulk_callback, agg); ret = usb_submit_urb(agg->urb, GFP_ATOMIC); For rx you could find the following code in r8152_submit_rx(). usb_fill_bulk_urb(agg->urb, tp->udev, usb_rcvbulkpipe(tp->udev, 1), agg->buffer, tp->rx_buf_sz, (usb_complete_t)read_bulk_callback, agg); ret = usb_submit_urb(agg->urb, mem_flags); > Some kind of timer ? > An (unrelated) incoming packet ? When the rx or tx is completed, a interrupt of USB host controller would be triggered. Then, the callback functions would be called. Best Regards, Hayes
Re: [PATCH net-next] r8152: divide the tx and rx bottom functions
On 8/16/19 11:08 AM, Hayes Wang wrote: > Eric Dumazet [mailto:eric.duma...@gmail.com] >> Sent: Friday, August 16, 2019 4:20 PM > [...] >> Which callback ? > > The USB device has two endpoints for Tx and Rx. > If I submit tx or rx URB to the USB host controller, > the relative callback functions would be called, when > they are finished. For rx, it is read_bulk_callback. > For tx, it is write_bulk_callback. > >> After an idle period (no activity, no prior packets being tx-completed ...), >> a packet is sent by the upper stack, enters the ndo_start_xmit() of a network >> driver. >> >> This driver ndo_start_xmit() simply adds an skb to a local list, and returns. > > Base on the current method (without tasklet), when > ndo_start_xmit() is called, napi_schedule is called only > if there is at least one free buffer (!list_empty(&tp->tx_free)) > to transmit the packet. Then, the flow would be as following. Very uncommon naming conventions really :/ Maybe you would avoid messing with a tasklet (we really try to get rid of tasklets in general) using two NAPI, one for TX, one for RX. Some drivers already use two NAPI, it is fine. This might avoid the ugly dance in r8152_poll(), calling napi_schedule(napi) after napi_complete_done() !
RE: [PATCH net-next] r8152: divide the tx and rx bottom functions
Eric Dumazet [mailto:eric.duma...@gmail.com] > Sent: Friday, August 16, 2019 5:27 PM [...] > Maybe you would avoid messing with a tasklet (we really try to get rid > of tasklets in general) using two NAPI, one for TX, one for RX. > > Some drivers already use two NAPI, it is fine. > > This might avoid the ugly dance in r8152_poll(), > calling napi_schedule(napi) after napi_complete_done() ! The reason is that the USB device couldn't control the interrupt of USB controller. That is, I couldn't disable the interrupt before napi_schedule and enable it after napi_complete_done. If the callback function occurs during the following timing, it is possible no one would schedule the napi again. static int r8152_poll(struct napi_struct *napi, int budget) { struct r8152 *tp = container_of(napi, struct r8152, napi); int work_done; work_done = rx_bottom(tp, budget); bottom_half(tp); --> callback occurs here and try to call napi_schedule napi_complete_done(napi, work_done) That is, no tx or rx could be handled unless something trigger napi_schedule. Best Regards, Hayes