[dpdk-dev] [EXT] Re: [PATCH 1/3] mbuf: add Tx offloads for packet marking

[Thomas Monjalon]

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).