Hi, For mlx4/mlx5 tx_burst we build hardware descriptors, push ones into hardware and write doorbell register. The doorbell register can be mapped by mlx4/mlx5 into user space either with non-cache or with write-combining (just regular attribute) memory attributes. The write-combining mapping requires the explicit memory barrier to push the written data to the destination hardware, it takes noticeable time and PMDs try to optimize out.
mlx4 just does not perform wmb after the doorbelling, it is always supposed it will happen on the next call. For mlx5 we have the very special Tx doorbelling mode (explicitly controlled via "tx_db_nc" devarg) to skip the last wmb in tx_burst routine. The user requesting this should understand all risks and take countermeasures in application if he/she cares about packet drops on queue/device stop. In the worst case, if wmb is postponed (even never happens anymore) it just causes the increased send latency for the packets in the last burst. If queue stop happens during the "non-promoted doorbell time period" the last burst packets might be dropped (and we suppose this is not crucial for service being terminated) To summarize: - mlx4 is in moderate risk of final send burst packet drop (we've never seen this in practice, just did not check though) - mlx5 is in minor risk of final send burst packet drop (in very special mode only, and we observed latency issues in practice), can be disregarded - barrier in dummy routine does not fully resolve this mlx specific issue (not invoked in guaranteed way on right core) My conclusion - I would prefer to keep barrier in dummy, "just for the case", but have no strong objections about removing, we can accept the patch being discussed. Acked-by: Viacheslav Ovsiienko <[email protected]> With best regards, Slava > -----Original Message----- > From: Andrew Rybchenko <[email protected]> > Sent: Thursday, February 10, 2022 13:52 > To: Morten Brørup <[email protected]>; Ferruh Yigit > <[email protected]>; Shepard Siegel <[email protected]>; > Ed Czeck <[email protected]>; John Miller > <[email protected]>; Rasesh Mody <[email protected]>; > Shahed Shaikh <[email protected]>; Ajit Khaparde > <[email protected]>; Somnath Kotur > <[email protected]>; Nithin Dabilpuram > <[email protected]>; Kiran Kumar K <[email protected]>; > Sunil Kumar Kori <[email protected]>; Satha Rao > <[email protected]>; Hemant Agrawal <[email protected]>; > Sachin Saxena <[email protected]>; John Daley > <[email protected]>; Hyong Youb Kim <[email protected]>; Min Hu > (Connor) <[email protected]>; Yisen Zhuang > <[email protected]>; Lijun Ou <[email protected]>; Matan Azrad > <[email protected]>; Slava Ovsiienko <[email protected]>; > Gagandeep Singh <[email protected]>; Devendra Singh Rawat > <[email protected]>; NBU-Contact-Thomas Monjalon (EXTERNAL) > <[email protected]> > Cc: [email protected]; Ciara Loftus <[email protected]> > Subject: Re: [PATCH] ethdev: introduce generic dummy packet burst function > > On 2/10/22 14:47, Morten Brørup wrote: > >> From: Andrew Rybchenko [mailto:[email protected]] > >> Sent: Thursday, 10 February 2022 12.39 > >> > >> On 2/10/22 14:04, Morten Brørup wrote: > >>>> From: Ferruh Yigit [mailto:[email protected]] > >>>> Sent: Tuesday, 8 February 2022 20.45 > >>>> > >>>> Multiple PMDs have dummy/noop Rx/Tx packet burst functions. > >>>> > >>>> These dummy functions are very simple, introduce a common function > >> in > >>>> the ethdev and update drivers to use it instead of each driver > >> having > >>>> its own functions. > >>>> > >>>> Signed-off-by: Ferruh Yigit <[email protected]> > >>> > >>> After briefly considering if the dummy TX should free the burst, I > >> concluded that the current behavior is correct. > >> > >> Could you share your thoughts, please. I'm wondering as well. > > > > Returning 0 means that the packets were not transmitted. > > > > This leaves it up to the application to decide what to do: drop or > > retransmit. > > > > If the dummy TX function frees the burst, it would effectively mean that the > driver dropped the packets. (In that case, some drop counters should > probably also be updated in the driver; but that is irrelevant now.) > > Makes sense, thank you. > > > > > Not dropping the packets could be significant during startup. > > > >> > >>> > >>> Good clean-up. :-) > >>> > >>> Acked-by: Morten Brørup <[email protected]> > >>> > >> > >
