On Mon, Dec 25, 2023 at 12:12:48PM +0800, Jason Wang wrote: > On Fri, Dec 22, 2023 at 4:14 PM Michael S. Tsirkin <mst@xxxxxxxxxx> wrote: > > > > On Fri, Dec 22, 2023 at 10:35:07AM +0800, Jason Wang wrote: > > > On Thu, Dec 21, 2023 at 11:06 PM Willem de Bruijn > > > <willemdebruijn.kernel@xxxxxxxxx> wrote: > > > > > > > > Heng Qi wrote: > > > > > > > > > > > > > > > 在 2023/12/20 下午10:45, Willem de Bruijn 写道: > > > > > > Heng Qi wrote: > > > > > >> virtio-net has two ways to switch napi_tx: one is through the > > > > > >> module parameter, and the other is through coalescing parameter > > > > > >> settings (provided that the nic status is down). > > > > > >> > > > > > >> Sometimes we face performance regression caused by napi_tx, > > > > > >> then we need to switch napi_tx when debugging. However, the > > > > > >> existing methods are a bit troublesome, such as needing to > > > > > >> reload the driver or turn off the network card. > > > > > > Why is this troublesome? We don't need to turn off the card, it's just > > > a toggling of the interface. > > > > > > This ends up with pretty simple code. > > > > > > > So try to make > > > > > >> this update. > > > > > >> > > > > > >> Signed-off-by: Heng Qi <hengqi@xxxxxxxxxxxxxxxxx> > > > > > >> Reviewed-by: Xuan Zhuo <xuanzhuo@xxxxxxxxxxxxxxxxx> > > > > > > The commit does not explain why it is safe to do so. > > > > > > > > > > virtnet_napi_tx_disable ensures that already scheduled tx napi ends and > > > > > no new tx napi will be scheduled. > > > > > > > > > > Afterwards, if the __netif_tx_lock_bh lock is held, the stack cannot > > > > > send the packet. > > > > > > > > > > Then we can safely toggle the weight to indicate where to clear the buffers. > > > > > > > > > > > > > > > > > The tx-napi weights are not really weights: it is a boolean whether > > > > > > napi is used for transmit cleaning, or whether packets are cleaned > > > > > > in ndo_start_xmit. > > > > > > > > > > Right. > > > > > > > > > > > > > > > > > There certainly are some subtle issues with regard to pausing/waking > > > > > > queues when switching between modes. > > > > > > > > > > What are "subtle issues" and if there are any, we find them. > > > > > > > > A single runtime test is not sufficient to exercise all edge cases. > > > > > > > > Please don't leave it to reviewers to establish the correctness of a > > > > patch. > > > > > > +1 > > > > > > And instead of trying to do this, it would be much better to optimize > > > the NAPI performance. Then we can drop the orphan mode. > > > > "To address your problem, optimize our code to the level which we > > couldn't achieve in more than 10 years". > > Last time QE didn't report any issue for TCP. For others, the code > might just need some optimization if it really matters, it's just > because nobody has worked on this part in the past years. You think nobody worked on performance of virtio net because nobody could bother? I think it's just micro optimized to a level where progress is difficult. > The ethtool trick is just for debugging purposes, I can hardly believe > it is used by any management layer software. It's UAPI - someone somewhere uses it, management layer or not. -- MST
