03/06/2020 13:38, Olivier Matz: > On Wed, Jun 03, 2020 at 04:14:14PM +0530, Nithin Dabilpuram wrote: > > On Wed, Jun 03, 2020 at 10:28:44AM +0200, Olivier Matz wrote: > > > On Tue, Jun 02, 2020 at 07:55:37PM +0530, Nithin Dabilpuram wrote: > > > > On Tue, Jun 02, 2020 at 10:53:08AM +0000, Ananyev, Konstantin wrote: > > > > > > > > > I also share Olivier's concern about consuming 3 bits in > > > > > > > > > ol_flags for that feature. > > > > > > > > > Can it probably be squeezed somehow? > > > > > > > > > Let say we reserve one flag that this information is present > > > > > > > > > or not, and > > > > > > > > > re-use one of rx-only fields for store additional information > > > > > > > > > (packet_type, or so). > > > > > > > > > Or might be some other approach. > > > > > > > > > > > > > > > > We are fine with this approach where we define one bit in Tx > > > > > > > > offloads for pkt > > > > > > > > marking and and 3 bits reused from Rx offload flags area. [...] > > > I'm not a big fan of reusing Rx fields or flags for Tx. > > > It's not obvious for an application than adding a tx_mark will overwrite > > > the packet_type. I understand that the risk is limited because packet_type > > > is Rx and the marks are Tx, but there is still one.
Mixing Rx and Tx info in the same field is a bad design pattern which will create a lot of difficult bugs. > > I'm also not a big fan but just wanted to take this approach so that, > > it can both conserve space and also help fast path. > > Reusing Rx area is however not a new thing as is already followed for > > mbuf->txadapter field. Yes there is a txadapter field union'ed with flow director and QoS. This is a bad pattern that I highlighted in this presentation (slide 8): https://www.dpdk.org/wp-content/uploads/sites/35/2019/10/DynamicMbuf.pdf > Yes, and in my opinion this is something we should avoid when possible, > because it makes some features exclusive (ex: the big union with > sched/rss/adapter/usr/...). Yes, the "RSS union" must be cleaned-up, as some other mbuf parts. > > Apart from documentation issue, Is there any other issue or future > > ramification with using Rx field's for Tx ? > > No, I don't see any other issue except the ones we already mentioned > (doc, code clarity, ). "doc clarity" should be understood as the opposite of "design leading inevitably to bugs". > > If it is only about documentation, then we can add more documentation to > > make things clear. More documentation won't make a bad design better, unfortunately. > > > To summarize the different proposed approaches (please correct me if I'm > > > wrong): > > > > > > a- add 3 Tx mbuf flags > > > (-) consumes limited resource > > > > > > b- add 3 dynamic flags > > > (-) slower > > > > - Tx burst Vector implementation can't be done for this tx offload as > > offset keeps changing. > > A vector implementation can be done. But yes, it would be slower than > with a static flag. > > > > c- add 1 Tx flag and union with Rx field > > > (-) exclusive with Rx field > > > (-) still consumes one flag > > > > > > My preference is still b-, for these reasons: Me too, my preference is (b).
