Re: [dpdk-dev] [PATCH v3 0/4] add AESNI-MB rawdev for multi-function processing

[Thomas Monjalon <thomas@monjalon.net>]

21/04/2020 20:37, Coyle, David:
> From: Thomas Monjalon <thomas@monjalon.net>
> > 21/04/2020 18:46, Doherty, Declan:
> > > On 15/04/2020 11:33 PM, Thomas Monjalon wrote:
> > > > 16/04/2020 00:19, Doherty, Declan:
> > > >> On 14/04/2020 3:44 PM, Thomas Monjalon wrote:
> > > >>> 14/04/2020 16:02, Trahe, Fiona:
> > > >>>> From: Thomas Monjalon <thomas@monjalon.net>
> > > >>>>> 14/04/2020 15:04, Trahe, Fiona:
> > > >>>>>>> 14/04/2020 12:21, Ferruh Yigit:
> > > >>>>>>>
> > > >>>>>
> > http://inbox.dpdk.org/dev/MN2PR11MB35507D4B96677A41E66440C5E3C30
> > > >>>>> @MN2PR11MB3550.na
> > > >>>>>>> mprd11.prod.outlook.com/
> > > >>>>>>>
> > > >>>>>>> I am not convinced.
> > > >>>>>>> I don't like rawdev in general.
> > > >>>>>>> Rawdev is good only for hardware support which cannot be
> > > >>>>>>> generic like SoC, FPGA management or DMA engine.
> > > >>>>>>
> > > >>>>>> [Fiona] CRC and BIP are not crypto algorithms, they are error
> > detection processes.
> > > >>>>>> So there is no class in DPDK that these readily fit into.
> > > >>>>>> There was resistance to adding another xxxddev, and even if one
> > > >>>>>> had been added for error_detection_dev, there would still have
> > > >>>>>> been another layer needed to couple this with cryptodev.
> > > >>>>>> Various proposals for this have been discussed on the ML in RFC
> > and recent patches, there doesn't seem to be an appetite for this as a
> > generic API.
> > > >>>>>> So it seems that only Intel has software and hardware engines
> > > >>>>>> that provide this specialised feature coupling. In that case
> > > >>>>>> rawdev seems like the most appropriate vehicle to expose this.
> > > >>>>>
> > > >>>>> Adding some vendor-specific API is not a good answer.
> > > >>>>> It will work in some cases, but it won't make DPDK better.
> > > >>>>> What's the purpose of DPDK if it's not solving a common problem
> > > >>>>> for different hardware?
> > > >>>>
> > > >> The current proposal in rawdev could easily be supported by any
> > > >> hardware which supports chaining multiple functions/services into a
> > > >> single operation, in this case symmetric crypto and error
> > > >> detection, but it could conceivably support chaining
> > > >> symmetric/asymmetric crypto operations or chaining symmetric crypto
> > and compression operations.
> > > >>
> > > >>>> [Fiona] Based on that logic rawdev should be deprecated.
> > > >>>> But the community has agreed that it has a place.
> > > >>>
> > > >>> No, as I said above, rawdev is good for SoC, FPGA management or
> > DMA engine.
> > > >>
> > > >> I distinctly remember when rawdev was being proposed one of the
> > > >> uses cases proposed was that a new classes of APIs could be
> > > >> prototyped and developed under rawdev and when a solid consensus
> > > >> was reached then migrated to a mainstream DPDK library. I think
> > > >> every effort has been made here to engage the community to develop
> > > >> a generic approach. As Fiona notes there hasn't really been much of an
> > appetite for this.
> > > >>
> > > >> Therefore I think the option to use rawdev makes sense, it allows
> > > >> an initial proposal to be deployed,  without a generic solution
> > > >> agreement, it will also give others in the community to see how
> > > >> this approach can work and hopefully lead to more engagement on a
> > > >> generic solution. Also as APIs in rawdev are essentially treated as
> > > >> private APIs the onus is on Intel to support this going forward.
> > > >
> > > > Because hardware support is pending, we should accept an Intel-only
> > > > "temporary" solution, opening the door to more vendor-specific APIs?
> > > >
> > > > What is the benefit for the DPDK project?
> > >
> > > Sorry I don't agree with this sentiment, David has made every attempt
> > > to solicit feedback an to engage the community in this.
> > 
> > Really?
> > 
> > These are the recipients of the first patch:
> > 	dev@dpdk.org, declan.doherty@intel.com, fiona.trahe@intel.com In
> > next patches, only Intel and NXP are Cc'ed.
> > Stephen and Jerin, who gave good comments on first patch, were not Cc'ed
> > in next versions.
> > 
> > Was it presented in an event?
> > Was it brought to the techboard?
> > Please don't exagerate and admit you are trying to push something which is
> > specific and convenient for Intel QuickAssist.
> 
> [DC] This is being brought to the TechBoard tomorrow (22/04)
> 
> > 
> > 
> > > I also don't agree in classifying this as a "temporary solution" as
> > > this is a solid proposal for an approach to chaining multiple
> > > operations together, but I guess the fact remains that we only
> > > currently have a single use-case, but it is difficult to generate a
> > > generic solution in this case.
> > >
> > > While there is only a single use case it is targeting two devices so
> > > that drove the need for a common interface withing rawdev.
> > >
> > > The advantage of using rawdev is that it allows this to be consumed
> > > through DPDK, which enables DPDK project consumers, but also leaves
> > > the door open to other contributors to have their say on how this
> > > should evolve. For example this exact process seems to be occurring
> > > with DMA engines in rawdev today, with a critical mass of
> > > implementations which now is giving the impetus to create a generic
> > > solution, as we would hope can occur here too in the future.
> > >
> > >
> > > >>>> And the common problem here is device exposure.
> > > >>>> With a specialised service on top.
> > > >>>>
> > > >>>>
> > > >>>>>>> Here the intent is to use rawdev because we don't find a good
> > API.
> > > >>>>>>> API defeat is a no-go.
> > > >>>>>>
> > > >>>>>> [Fiona] It's not that we haven't found a good API, but that
> > > >>>>>> there doesn't seem to be a general requirement for such a
> > > >>>>>> specialised API
> > > >>>>>
> > > >>>>> There is a requirement to combine features of different classes.
> > > >>>>
> > > >>>> [Fiona] Can you point me to that requirement please?
> > > >>>
> > > >>> Yes, rte_security is trying to address this exact issue.
> > > >>>
> > > >>
> > > >> I don't agree rte_security addresses the problem of different
> > > >> device types supporting the same services. The problem being
> > > >> addressed here is a single device which supports the chaining of
> > > >> multiple services (sym crypto & error detection)
> > > >
> > > > Doing IPsec processing in Rx or Tx of a NIC is not chaining?
> > >
> > > I wouldn't consider an inline crypto offload or full IPsec offload a
> > > chained operation in the vein being proposed here where completely
> > > independent services (in the view of DPDK which are currently on
> > > independent devices and APIs) are linked together.
> > >
> > > We did look at using rte_security here but it wasn't considered
> > > suitable for a chaining of non-crypto operations such as CRC or
> > > possibly compression in the future, as it would still run into the
> > > issue of having to use the cryptodev enq/deq API in the lookaside offload
> > case.
> > 
> > Because rte_security is not a generic solution (that's why I don't like it).
> > I think a good approach would be to check how to offload in HW the chaining
> > done in frameworks like rte_pipeline or rte_graph.
> > Stephen and Jerin already talked about it, but it was rejected by David,
> > because harder to implement I think.
> > Even worst, the team working on this patch did zero review of rte_graph.
> 
> [DC] The team working on this patch did review rte_graph and explained our reasoning
> to Jerin as to why we felt it was not suitable.

No, by review, I mean doing some comments in the rte_graph series
to help making the code, design or doc better.
You are complaining about the lack of attention to your patch,
that's why I point out that you didn't help other patches to progress.

> While Jerin explained it would be possible to combine 2 nodes as a single optimized node at runtime,
> he also agreed that it did NOT make sense to abstract what we are trying to do as a graph.

I think this exact point requires more discussion.

> Please see http://mails.dpdk.org/archives/dev/2020-March/159238.html

He did not say "it does not make sense" but
"In that way, it makes sense to not abstract as a graph."
This is slightly different.
Anyway, we should re-consider using a graph abstraction for chaining.

> > I think the chaining requirement is a real problem to solve, and it deserves a
> > good architecture and API.
> > Maybe this future API should be based on rte_graph.