On Mon, Dec 25, 2023 at 4:03 PM Michael S. Tsirkin <mst@xxxxxxxxxx> wrote: > > 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? No, I just describe what I've seen from the list. No patches were posted for performance optimization in recent years. > I think it's just micro optimized to a level where > progress is difficult. > > Maybe. Let me clarify what I meant. If people can help to optimize the NAPI to be as fast as orphans or something like 80%-90% of orphans. It should be sufficient to remove the orphan path completely. This allows a lot of good features to be built easily on top. To me, it seems fine to remove it even now as it breaks socket accounting which is actually a bug and NAPI has been enabled for years but if you don't wish to do that, that's fine. We can wait for the coalescing and dim to be used for a while and revisit this. Thanks
