Re: [dpdk-dev] [PATCH] compressdev: implement API
> -Original Message- > From: Thomas Monjalon [mailto:tho...@monjalon.net] > Sent: Saturday, March 24, 2018 1:02 AM > To: Trahe, Fiona > Cc: dev@dpdk.org; ahmed.mans...@nxp.com; shally.ve...@cavium.com; De Lara > Guarch, Pablo > ; fiona.tr...@gmail.com > Subject: Re: [dpdk-dev] [PATCH] compressdev: implement API > > 23/03/2018 19:08, Trahe, Fiona: > > From: Thomas Monjalon [mailto:tho...@monjalon.net] > > > 02/02/2018 19:25, Fiona Trahe: > > > > lib/librte_compressdev/rte_comp.h | 503 > > > > > > Why rte_comp.h instead of the more consistent rte_compress.h? > > [Fiona] I did originally... but ran into difficulty with horribly names > > like > > RTE_COMPRESS_COMPRESS > > RTE_COMPRESS_DECOMPRESS > > rte_compress_compress_xform > > rte_compress_decompress_xform > > So compress is both the module prefix and the name of one of the actions. > > I could have used compressdev - but names were very long. > > So decided to opt for using > > _compressdev_ in names to do with the device and > > _comp_ in names to do with the compression service > > > > Also I could have used compdev instead of compressdev, > > but I felt compress should be in the lib name > > I understand your concerns. > I don't like "comp" very much because it sounds like "comparison". > However, I don't have a better idea. > Sometimes naming is more difficult than coding :) [Fiona] True :)
Re: [dpdk-dev] [PATCH] compressdev: implement API
23/03/2018 19:08, Trahe, Fiona: > From: Thomas Monjalon [mailto:tho...@monjalon.net] > > 02/02/2018 19:25, Fiona Trahe: > > > lib/librte_compressdev/rte_comp.h | 503 > > > > Why rte_comp.h instead of the more consistent rte_compress.h? > [Fiona] I did originally... but ran into difficulty with horribly names like > RTE_COMPRESS_COMPRESS > RTE_COMPRESS_DECOMPRESS > rte_compress_compress_xform > rte_compress_decompress_xform > So compress is both the module prefix and the name of one of the actions. > I could have used compressdev - but names were very long. > So decided to opt for using > _compressdev_ in names to do with the device and > _comp_ in names to do with the compression service > > Also I could have used compdev instead of compressdev, > but I felt compress should be in the lib name I understand your concerns. I don't like "comp" very much because it sounds like "comparison". However, I don't have a better idea. Sometimes naming is more difficult than coding :)
Re: [dpdk-dev] [PATCH] compressdev: implement API
Hi Tomas, Sorry for the delay in replying to this > -Original Message- > From: Thomas Monjalon [mailto:tho...@monjalon.net] > Sent: Sunday, February 4, 2018 2:25 PM > To: Trahe, Fiona > Cc: dev@dpdk.org; ahmed.mans...@nxp.com; shally.ve...@cavium.com; De Lara > Guarch, Pablo > > Subject: Re: [dpdk-dev] [PATCH] compressdev: implement API > > 02/02/2018 19:25, Fiona Trahe: > > config/common_base | 6 + > > doc/api/doxy-api-index.md | 1 + > > doc/api/doxy-api.conf | 1 + > > lib/Makefile | 3 + > > lib/librte_compressdev/Makefile| 29 + > > lib/librte_compressdev/rte_comp.h | 503 > > Why rte_comp.h instead of the more consistent rte_compress.h? [Fiona] I did originally... but ran into difficulty with horribly names like RTE_COMPRESS_COMPRESS RTE_COMPRESS_DECOMPRESS rte_compress_compress_xform rte_compress_decompress_xform So compress is both the module prefix and the name of one of the actions. I could have used compressdev - but names were very long. So decided to opt for using _compressdev_ in names to do with the device and _comp_ in names to do with the compression service Also I could have used compdev instead of compressdev, but I felt compress should be in the lib name > > > lib/librte_compressdev/rte_compressdev.c | 902 > > + > > lib/librte_compressdev/rte_compressdev.h | 757 + > > lib/librte_compressdev/rte_compressdev_pmd.c | 163 > > lib/librte_compressdev/rte_compressdev_pmd.h | 439 ++ > > lib/librte_compressdev/rte_compressdev_version.map | 47 ++ > > lib/librte_eal/common/include/rte_log.h| 1 + > > mk/rte.app.mk | 1 + > > 13 files changed, 2853 insertions(+) > > Please update MAINTAINERS file and release notes. > > Maybe it is worth splitting this patch in few shorter parts? [Fiona] will do > > > > --- a/config/common_base > > +++ b/config/common_base > > @@ -535,6 +535,12 @@ CONFIG_RTE_LIBRTE_PMD_MRVL_CRYPTO=n > > CONFIG_RTE_LIBRTE_PMD_MRVL_CRYPTO_DEBUG=n > > > > # > > +# Compile generic compression device library > > +# > > +CONFIG_RTE_LIBRTE_COMPRESSDEV=y > > +CONFIG_RTE_COMPRESS_MAX_DEVS=64 > > + > > +# > > # Compile generic security library > > # > > CONFIG_RTE_LIBRTE_SECURITY=y > > diff --git a/doc/api/doxy-api-index.md b/doc/api/doxy-api-index.md > > index d77f205..07b8e75 100644 > > --- a/doc/api/doxy-api-index.md > > +++ b/doc/api/doxy-api-index.md > > @@ -43,6 +43,7 @@ The public API headers are grouped by topics: > >[rte_tm] (@ref rte_tm.h), > >[rte_mtr](@ref rte_mtr.h), > >[bbdev] (@ref rte_bbdev.h), > > + [compressdev](@ref rte_compressdev.h), > >[cryptodev] (@ref rte_cryptodev.h), > >[security] (@ref rte_security.h), > >[eventdev] (@ref rte_eventdev.h), > > Please move it between security and eventdev in these lists. ok
Re: [dpdk-dev] [PATCH] compressdev: implement API - mbuf alternative
> -Original Message- > From: Verma, Shally [mailto:shally.ve...@cavium.com] > Sent: Thursday, March 15, 2018 4:12 AM > To: Ahmed Mansour ; Trahe, Fiona > ; dev@dpdk.org > Cc: De Lara Guarch, Pablo ; Athreya, Narayana > Prasad > ; Gupta, Ashish ; > Sahu, Sunila > ; Challa, Mahipal ; Jain, > Deepak K > ; Hemant Agrawal ; Roy Pledge > ; Youri Querry ; Daly, Lee > ; > Jozwiak, TomaszX ; Alok Makhariya > ; Shreyansh > Jain > Subject: RE: [dpdk-dev] [PATCH] compressdev: implement API - mbuf alternative > > @Trahe, Fiona>> We're proposing, in the interest of getting the API out in > 18.05, to stick with mbufs - > acknowledging > >> that they're not optimal for storage and we may propose changes in 18.08. > [Shally] Sounds good to us too. > > @Ahmed Mansour . I am assuming that transferring from mbuf to regular buffers > and back does > >not involve some time consuming work like data copying and such. > [Shally] I too assume copying shouldn't be a need and a big no-no. We > normally extract and pass buf_addr > from mbuf as it is to HW. > So implicit assumption is data memory is dma-able to device. [Fiona] agreed > > Thanks > Shally > > >-Original Message- > >From: Ahmed Mansour [mailto:ahmed.mans...@nxp.com] > >Sent: 15 March 2018 00:32 > >To: Trahe, Fiona ; Verma, Shally > >; dev@dpdk.org > >Cc: De Lara Guarch, Pablo ; Athreya, > >Narayana Prasad > ; > >Gupta, Ashish ; Sahu, Sunila > >; Challa, Mahipal > >; Jain, Deepak K ; > >Hemant Agrawal > ; Roy > >Pledge ; Youri Querry ; Daly, Lee > ; Jozwiak, TomaszX > >; Alok Makhariya ; > >Shreyansh Jain > > >Subject: Re: [dpdk-dev] [PATCH] compressdev: implement API - mbuf alternative > > > >Hi All, > > > >Sticking with mbufs until at least 1805 works for us. We also see > >storage as the main use case, but ipcomp maybe an important customer use > >case in the future. Nonetheless, I see the mbuf formatting as inherently > >external to the compressdev APIs. An application doing ipcomp should > >just do the mbuf packaging outside of compressdev. At least that is what > >current software implementation of ipcomp do when using zlib.net. I am > >assuming that transferring from mbuf to regular buffers and back does > >not involve some time consuming work like data copying and such. > > > >Thanks, > > > >Ahmed > > > >On 3/14/2018 2:39 PM, Trahe, Fiona wrote: > >> Hi Shally, Ahmed, et al, > >> > >> Following internal and community feedback we've decided that there's still > >> too much churn in this. > >> We're proposing, in the interest of getting the API out in 18.05, to stick > >> with mbufs - acknowledging > >> that they're not optimal for storage and we may propose changes in 18.08. > >> Compressdev will start as an experimental API in 18.05 - we'll POC and > >> benchmark alternatives > >> or API extensions once we get time to do so. > >> > >> Regards, > >> Fiona > >> > >>> -Original Message- > >>> From: Verma, Shally [mailto:shally.ve...@cavium.com] > >>> Sent: Wednesday, March 14, 2018 12:51 PM > >>> To: Trahe, Fiona ; Ahmed Mansour > >>> ; > dev@dpdk.org > >>> Cc: De Lara Guarch, Pablo ; Athreya, > >>> Narayana Prasad > >>> ; Gupta, Ashish > >>> ; Sahu, Sunila > >>> ; Challa, Mahipal ; > >>> Jain, Deepak K > >>> ; Hemant Agrawal ; Roy > >>> Pledge > >>> ; Youri Querry ; Daly, Lee > >>> ; > >>> Jozwiak, TomaszX > >>> Subject: RE: [dpdk-dev] [PATCH] compressdev: implement API - mbuf > >>> alternative > >>> > >>> > >>> > >>>> -Original Message- > >>>> From: Trahe, Fiona [mailto:fiona.tr...@intel.com] > >>>> Sent: 13 March 2018 21:22 > >>>> To: Verma, Shally ; Ahmed Mansour > >>>> ; > >>> dev@dpdk.org > >>>> Cc: De Lara Guarch, Pablo ; Athreya, > >>>> Narayana Prasad > >>> ; > >>>> Gupta, Ashish ; Sahu, Sunila > >>>> ; Challa, > Mahipal > >>>> ; Jain, Deepak K ; > >>>> Hemant Agrawal > >>> ; Roy > >>>> Pledge ; Youri Querry ; > >>>> Daly, Lee > >>> ; Jozwiak, TomaszX > >>>> ; Trahe, Fiona > >>>
Re: [dpdk-dev] [PATCH] compressdev: implement API - mbuf alternative
@Trahe, Fiona>> We're proposing, in the interest of getting the API out in 18.05, to stick with mbufs - acknowledging >> that they're not optimal for storage and we may propose changes in 18.08. [Shally] Sounds good to us too. @Ahmed Mansour . I am assuming that transferring from mbuf to regular buffers and back does >not involve some time consuming work like data copying and such. [Shally] I too assume copying shouldn't be a need and a big no-no. We normally extract and pass buf_addr from mbuf as it is to HW. So implicit assumption is data memory is dma-able to device. Thanks Shally >-Original Message- >From: Ahmed Mansour [mailto:ahmed.mans...@nxp.com] >Sent: 15 March 2018 00:32 >To: Trahe, Fiona ; Verma, Shally >; dev@dpdk.org >Cc: De Lara Guarch, Pablo ; Athreya, Narayana >Prasad ; >Gupta, Ashish ; Sahu, Sunila >; Challa, Mahipal >; Jain, Deepak K ; Hemant >Agrawal ; Roy >Pledge ; Youri Querry ; Daly, Lee >; Jozwiak, TomaszX >; Alok Makhariya ; >Shreyansh Jain >Subject: Re: [dpdk-dev] [PATCH] compressdev: implement API - mbuf alternative > >Hi All, > >Sticking with mbufs until at least 1805 works for us. We also see >storage as the main use case, but ipcomp maybe an important customer use >case in the future. Nonetheless, I see the mbuf formatting as inherently >external to the compressdev APIs. An application doing ipcomp should >just do the mbuf packaging outside of compressdev. At least that is what >current software implementation of ipcomp do when using zlib.net. I am >assuming that transferring from mbuf to regular buffers and back does >not involve some time consuming work like data copying and such. > >Thanks, > >Ahmed > >On 3/14/2018 2:39 PM, Trahe, Fiona wrote: >> Hi Shally, Ahmed, et al, >> >> Following internal and community feedback we've decided that there's still >> too much churn in this. >> We're proposing, in the interest of getting the API out in 18.05, to stick >> with mbufs - acknowledging >> that they're not optimal for storage and we may propose changes in 18.08. >> Compressdev will start as an experimental API in 18.05 - we'll POC and >> benchmark alternatives >> or API extensions once we get time to do so. >> >> Regards, >> Fiona >> >>> -Original Message- >>> From: Verma, Shally [mailto:shally.ve...@cavium.com] >>> Sent: Wednesday, March 14, 2018 12:51 PM >>> To: Trahe, Fiona ; Ahmed Mansour >>> ; dev@dpdk.org >>> Cc: De Lara Guarch, Pablo ; Athreya, >>> Narayana Prasad >>> ; Gupta, Ashish >>> ; Sahu, Sunila >>> ; Challa, Mahipal ; >>> Jain, Deepak K >>> ; Hemant Agrawal ; Roy >>> Pledge >>> ; Youri Querry ; Daly, Lee >>> ; >>> Jozwiak, TomaszX >>> Subject: RE: [dpdk-dev] [PATCH] compressdev: implement API - mbuf >>> alternative >>> >>> >>> >>>> -Original Message- >>>> From: Trahe, Fiona [mailto:fiona.tr...@intel.com] >>>> Sent: 13 March 2018 21:22 >>>> To: Verma, Shally ; Ahmed Mansour >>>> ; >>> dev@dpdk.org >>>> Cc: De Lara Guarch, Pablo ; Athreya, >>>> Narayana Prasad >>> ; >>>> Gupta, Ashish ; Sahu, Sunila >>>> ; Challa, Mahipal >>>> ; Jain, Deepak K ; >>>> Hemant Agrawal >>> ; Roy >>>> Pledge ; Youri Querry ; Daly, >>>> Lee >>> ; Jozwiak, TomaszX >>>> ; Trahe, Fiona >>>> Subject: RE: [dpdk-dev] [PATCH] compressdev: implement API - mbuf >>>> alternative >>>> >>>> Hi Shally, >>>> >>>>> -Original Message- >>>>> From: Verma, Shally [mailto:shally.ve...@cavium.com] >>>>> Sent: Tuesday, March 13, 2018 8:15 AM >>>>> To: Trahe, Fiona ; Ahmed Mansour >>>>> ; >>> dev@dpdk.org >>>>> Cc: De Lara Guarch, Pablo ; Athreya, >>>>> Narayana Prasad >>>>> ; Gupta, Ashish >>>>> ; Sahu, Sunila >>>>> ; Challa, Mahipal ; >>>>> Jain, Deepak K >>>>> ; Hemant Agrawal ; Roy >>>>> Pledge >>>>> ; Youri Querry ; >>>>> fiona.tr...@gmail.com; Daly, Lee >>>>> ; Jozwiak, TomaszX >>>>> Subject: RE: [dpdk-dev] [PATCH] compressdev: implement API - mbuf >>>>> alternative >>>>> >>>>> HI Fiona >>>>> >>>>> So I understand we'r
Re: [dpdk-dev] [PATCH] compressdev: implement API - mbuf alternative
Hi All, Sticking with mbufs until at least 1805 works for us. We also see storage as the main use case, but ipcomp maybe an important customer use case in the future. Nonetheless, I see the mbuf formatting as inherently external to the compressdev APIs. An application doing ipcomp should just do the mbuf packaging outside of compressdev. At least that is what current software implementation of ipcomp do when using zlib.net. I am assuming that transferring from mbuf to regular buffers and back does not involve some time consuming work like data copying and such. Thanks, Ahmed On 3/14/2018 2:39 PM, Trahe, Fiona wrote: > Hi Shally, Ahmed, et al, > > Following internal and community feedback we've decided that there's still > too much churn in this. > We're proposing, in the interest of getting the API out in 18.05, to stick > with mbufs - acknowledging > that they're not optimal for storage and we may propose changes in 18.08. > Compressdev will start as an experimental API in 18.05 - we'll POC and > benchmark alternatives > or API extensions once we get time to do so. > > Regards, > Fiona > >> -Original Message- >> From: Verma, Shally [mailto:shally.ve...@cavium.com] >> Sent: Wednesday, March 14, 2018 12:51 PM >> To: Trahe, Fiona ; Ahmed Mansour >> ; dev@dpdk.org >> Cc: De Lara Guarch, Pablo ; Athreya, >> Narayana Prasad >> ; Gupta, Ashish >> ; Sahu, Sunila >> ; Challa, Mahipal ; Jain, >> Deepak K >> ; Hemant Agrawal ; Roy >> Pledge >> ; Youri Querry ; Daly, Lee >> ; >> Jozwiak, TomaszX >> Subject: RE: [dpdk-dev] [PATCH] compressdev: implement API - mbuf alternative >> >> >> >>> -Original Message- >>> From: Trahe, Fiona [mailto:fiona.tr...@intel.com] >>> Sent: 13 March 2018 21:22 >>> To: Verma, Shally ; Ahmed Mansour >>> ; >> dev@dpdk.org >>> Cc: De Lara Guarch, Pablo ; Athreya, >>> Narayana Prasad >> ; >>> Gupta, Ashish ; Sahu, Sunila >>> ; Challa, Mahipal >>> ; Jain, Deepak K ; >>> Hemant Agrawal >> ; Roy >>> Pledge ; Youri Querry ; Daly, >>> Lee >> ; Jozwiak, TomaszX >>> ; Trahe, Fiona >>> Subject: RE: [dpdk-dev] [PATCH] compressdev: implement API - mbuf >>> alternative >>> >>> Hi Shally, >>> >>>> -Original Message- >>>> From: Verma, Shally [mailto:shally.ve...@cavium.com] >>>> Sent: Tuesday, March 13, 2018 8:15 AM >>>> To: Trahe, Fiona ; Ahmed Mansour >>>> ; >> dev@dpdk.org >>>> Cc: De Lara Guarch, Pablo ; Athreya, >>>> Narayana Prasad >>>> ; Gupta, Ashish >>>> ; Sahu, Sunila >>>> ; Challa, Mahipal ; >>>> Jain, Deepak K >>>> ; Hemant Agrawal ; Roy >>>> Pledge >>>> ; Youri Querry ; >>>> fiona.tr...@gmail.com; Daly, Lee >>>> ; Jozwiak, TomaszX >>>> Subject: RE: [dpdk-dev] [PATCH] compressdev: implement API - mbuf >>>> alternative >>>> >>>> HI Fiona >>>> >>>> So I understand we're moving away from mbufs because of its size >>>> limitation (64k) and cacheline >> overhead >>>> and their more suitability to n/w applications. Given that, I understand >>>> benefit of having another >> structure >>>> to input data but then what is proposal for ipcomp like application where >>>> mbuf usage may be a better >>>> option? Should we keep support for both (mbuf and this structure) so that >>>> apps can use appropriate >> data >>>> structure depending on their requirement. >>> [Fiona] An application can use pass buffers from an mbuf or mbuf chain to >>> compressdev by filling in the >>> compressdev struct fields with the mbuf meta-data, using >>> rte_pktmbuf_data_len(), >>> rte_pktmbuf_mtod(), rte_pktmbuf_mtophys(), etc >>> For simplicity I'd prefer to offer only 1 rather than 2 data formats on the >>> API. >>> We see storage applications rather than IPComp as the main use-case for >>> compressdev, so would prefer >>> to optimise for that. >>> Do you think otherwise? >> [Shally] Yea. We plan to use it for ipcomp and other such possible n/w apps. >> So, we envision mbuf support >> as necessary. So, I think we can add two APIs one which process on >> rte_comp_op and other on rte_mbufs >> to make it simpler. >> >>>> Further comments, on github. >
Re: [dpdk-dev] [PATCH] compressdev: implement API - mbuf alternative
Hi Shally, Ahmed, et al, Following internal and community feedback we've decided that there's still too much churn in this. We're proposing, in the interest of getting the API out in 18.05, to stick with mbufs - acknowledging that they're not optimal for storage and we may propose changes in 18.08. Compressdev will start as an experimental API in 18.05 - we'll POC and benchmark alternatives or API extensions once we get time to do so. Regards, Fiona > -Original Message- > From: Verma, Shally [mailto:shally.ve...@cavium.com] > Sent: Wednesday, March 14, 2018 12:51 PM > To: Trahe, Fiona ; Ahmed Mansour > ; dev@dpdk.org > Cc: De Lara Guarch, Pablo ; Athreya, Narayana > Prasad > ; Gupta, Ashish ; > Sahu, Sunila > ; Challa, Mahipal ; Jain, > Deepak K > ; Hemant Agrawal ; Roy Pledge > ; Youri Querry ; Daly, Lee > ; > Jozwiak, TomaszX > Subject: RE: [dpdk-dev] [PATCH] compressdev: implement API - mbuf alternative > > > > >-Original Message- > >From: Trahe, Fiona [mailto:fiona.tr...@intel.com] > >Sent: 13 March 2018 21:22 > >To: Verma, Shally ; Ahmed Mansour > >; > dev@dpdk.org > >Cc: De Lara Guarch, Pablo ; Athreya, > >Narayana Prasad > ; > >Gupta, Ashish ; Sahu, Sunila > >; Challa, Mahipal > >; Jain, Deepak K ; > >Hemant Agrawal > ; Roy > >Pledge ; Youri Querry ; Daly, Lee > ; Jozwiak, TomaszX > >; Trahe, Fiona > >Subject: RE: [dpdk-dev] [PATCH] compressdev: implement API - mbuf alternative > > > >Hi Shally, > > > >> -Original Message- > >> From: Verma, Shally [mailto:shally.ve...@cavium.com] > >> Sent: Tuesday, March 13, 2018 8:15 AM > >> To: Trahe, Fiona ; Ahmed Mansour > >> ; > dev@dpdk.org > >> Cc: De Lara Guarch, Pablo ; Athreya, > >> Narayana Prasad > >> ; Gupta, Ashish > >> ; Sahu, Sunila > >> ; Challa, Mahipal ; > >> Jain, Deepak K > >> ; Hemant Agrawal ; Roy > >> Pledge > >> ; Youri Querry ; > >> fiona.tr...@gmail.com; Daly, Lee > >> ; Jozwiak, TomaszX > >> Subject: RE: [dpdk-dev] [PATCH] compressdev: implement API - mbuf > >> alternative > >> > >> HI Fiona > >> > >> So I understand we're moving away from mbufs because of its size > >> limitation (64k) and cacheline > overhead > >> and their more suitability to n/w applications. Given that, I understand > >> benefit of having another > structure > >> to input data but then what is proposal for ipcomp like application where > >> mbuf usage may be a better > >> option? Should we keep support for both (mbuf and this structure) so that > >> apps can use appropriate > data > >> structure depending on their requirement. > >[Fiona] An application can use pass buffers from an mbuf or mbuf chain to > >compressdev by filling in the > >compressdev struct fields with the mbuf meta-data, using > >rte_pktmbuf_data_len(), > >rte_pktmbuf_mtod(), rte_pktmbuf_mtophys(), etc > >For simplicity I'd prefer to offer only 1 rather than 2 data formats on the > >API. > >We see storage applications rather than IPComp as the main use-case for > >compressdev, so would prefer > >to optimise for that. > >Do you think otherwise? > > [Shally] Yea. We plan to use it for ipcomp and other such possible n/w apps. > So, we envision mbuf support > as necessary. So, I think we can add two APIs one which process on > rte_comp_op and other on rte_mbufs > to make it simpler. > > > > >> > >> Further comments, on github. > >> > >> Thanks > >> Shally > >> > >> >-Original Message- > >> >From: Trahe, Fiona [mailto:fiona.tr...@intel.com] > >> >Sent: 12 March 2018 21:31 > >> >To: Ahmed Mansour ; Verma, Shally > >> >; > >> dev@dpdk.org > >> >Cc: De Lara Guarch, Pablo ; Athreya, > >> >Narayana Prasad > >> ; > >> >Gupta, Ashish ; Sahu, Sunila > >> >; Challa, > Mahipal > >> >; Jain, Deepak K ; > >> >Hemant Agrawal > >> ; Roy > >> >Pledge ; Youri Querry ; > >> >fiona.tr...@gmail.com; > Daly, > >> Lee ; > >> >Jozwiak, TomaszX > >> >Subject: RE: [dpdk-dev] [PATCH] compressdev: implement API - mbuf > >> >alternative > >> > > >> >Hi Shally, Ahmed, and anyone else interested in compressdev, > >> > > >> >I mentioned last week tha
Re: [dpdk-dev] [PATCH] compressdev: implement API - mbuf alternative
>-Original Message- >From: Trahe, Fiona [mailto:fiona.tr...@intel.com] >Sent: 13 March 2018 21:22 >To: Verma, Shally ; Ahmed Mansour >; dev@dpdk.org >Cc: De Lara Guarch, Pablo ; Athreya, Narayana >Prasad ; >Gupta, Ashish ; Sahu, Sunila >; Challa, Mahipal >; Jain, Deepak K ; Hemant >Agrawal ; Roy >Pledge ; Youri Querry ; Daly, Lee >; Jozwiak, TomaszX >; Trahe, Fiona >Subject: RE: [dpdk-dev] [PATCH] compressdev: implement API - mbuf alternative > >Hi Shally, > >> -Original Message- >> From: Verma, Shally [mailto:shally.ve...@cavium.com] >> Sent: Tuesday, March 13, 2018 8:15 AM >> To: Trahe, Fiona ; Ahmed Mansour >> ; dev@dpdk.org >> Cc: De Lara Guarch, Pablo ; Athreya, >> Narayana Prasad >> ; Gupta, Ashish >> ; Sahu, Sunila >> ; Challa, Mahipal ; Jain, >> Deepak K >> ; Hemant Agrawal ; Roy >> Pledge >> ; Youri Querry ; >> fiona.tr...@gmail.com; Daly, Lee >> ; Jozwiak, TomaszX >> Subject: RE: [dpdk-dev] [PATCH] compressdev: implement API - mbuf alternative >> >> HI Fiona >> >> So I understand we're moving away from mbufs because of its size limitation >> (64k) and cacheline overhead >> and their more suitability to n/w applications. Given that, I understand >> benefit of having another structure >> to input data but then what is proposal for ipcomp like application where >> mbuf usage may be a better >> option? Should we keep support for both (mbuf and this structure) so that >> apps can use appropriate data >> structure depending on their requirement. >[Fiona] An application can use pass buffers from an mbuf or mbuf chain to >compressdev by filling in the >compressdev struct fields with the mbuf meta-data, using >rte_pktmbuf_data_len(), >rte_pktmbuf_mtod(), rte_pktmbuf_mtophys(), etc >For simplicity I'd prefer to offer only 1 rather than 2 data formats on the >API. >We see storage applications rather than IPComp as the main use-case for >compressdev, so would prefer >to optimise for that. >Do you think otherwise? [Shally] Yea. We plan to use it for ipcomp and other such possible n/w apps. So, we envision mbuf support as necessary. So, I think we can add two APIs one which process on rte_comp_op and other on rte_mbufs to make it simpler. > >> >> Further comments, on github. >> >> Thanks >> Shally >> >> >-Original Message- >> >From: Trahe, Fiona [mailto:fiona.tr...@intel.com] >> >Sent: 12 March 2018 21:31 >> >To: Ahmed Mansour ; Verma, Shally >> >; >> dev@dpdk.org >> >Cc: De Lara Guarch, Pablo ; Athreya, >> >Narayana Prasad >> ; >> >Gupta, Ashish ; Sahu, Sunila >> >; Challa, Mahipal >> >; Jain, Deepak K ; >> >Hemant Agrawal >> ; Roy >> >Pledge ; Youri Querry ; >> >fiona.tr...@gmail.com; Daly, >> Lee ; >> >Jozwiak, TomaszX >> >Subject: RE: [dpdk-dev] [PATCH] compressdev: implement API - mbuf >> >alternative >> > >> >Hi Shally, Ahmed, and anyone else interested in compressdev, >> > >> >I mentioned last week that we've been exploring using something other than >> >mbufs to pass src/dst >> buffers to compressdev PMDs. >> > >> >Reasons: >> > - mbuf data is limited to 64k-1 in each segment of a chained mbuf. Data >> > for compression >> >can be greater and it would add cycles to have to break up into smaller >> > segments. >> > - data may originate in mbufs, but is more likely, particularly for >> > storage use-cases, to >> >originate in other data structures. >> > - There's a 2 cache-line overhead for every segment in a chain, most of >> > this data >> >is network-related, not needed by compressdev >> >So moving to a custom structure would minimise memory overhead, remove >> >restriction on 64k-1 size and >> give more flexibility if >> >compressdev ever needs any comp-specific meta-data. >> > >> >We've come up with a compressdev-specific structure using the struct iovec >> >from sys/uio.h, which is >> commonly used by storage >> >applications. This would replace the src and dest mbufs in the op. >> >I'll not include the code here - Pablo will push that to github shortly and >> >we'd appreciate review >> comments there. >> >https://github.com/pablodelara/dpdk-draft-compressdev >> >Just posting on the mailing list to give a heads-up and ensure this reaches >> >a wider audience than may see >> it on github. >> > >> >Note : We also considered having no data structures in the op, instead the >> >application >> >would supply a callback which the PMD would use to retrieve meta-data (virt >> >address, iova, length) >> >for each next segment as needed. While this is quite flexible and allow the >> >application >> >to keep its data in its native structures, it's likely to cost more cycles. >> >So we're not proposing this at the moment, but hope to benchmark it later >> >while the API is still >> experimental. >> > >> >General feedback on direction is welcome here on the mailing list. >> >For feedback on the details of implementation we would appreciate comments >> >on github. >> > >> >Regards, >> >Fiona.
Re: [dpdk-dev] [PATCH] compressdev: implement API - mbuf alternative
Hi Shally, > -Original Message- > From: Verma, Shally [mailto:shally.ve...@cavium.com] > Sent: Tuesday, March 13, 2018 8:15 AM > To: Trahe, Fiona ; Ahmed Mansour > ; dev@dpdk.org > Cc: De Lara Guarch, Pablo ; Athreya, Narayana > Prasad > ; Gupta, Ashish ; > Sahu, Sunila > ; Challa, Mahipal ; Jain, > Deepak K > ; Hemant Agrawal ; Roy Pledge > ; Youri Querry ; > fiona.tr...@gmail.com; Daly, Lee > ; Jozwiak, TomaszX > Subject: RE: [dpdk-dev] [PATCH] compressdev: implement API - mbuf alternative > > HI Fiona > > So I understand we're moving away from mbufs because of its size limitation > (64k) and cacheline overhead > and their more suitability to n/w applications. Given that, I understand > benefit of having another structure > to input data but then what is proposal for ipcomp like application where > mbuf usage may be a better > option? Should we keep support for both (mbuf and this structure) so that > apps can use appropriate data > structure depending on their requirement. [Fiona] An application can use pass buffers from an mbuf or mbuf chain to compressdev by filling in the compressdev struct fields with the mbuf meta-data, using rte_pktmbuf_data_len(), rte_pktmbuf_mtod(), rte_pktmbuf_mtophys(), etc For simplicity I'd prefer to offer only 1 rather than 2 data formats on the API. We see storage applications rather than IPComp as the main use-case for compressdev, so would prefer to optimise for that. Do you think otherwise? > > Further comments, on github. > > Thanks > Shally > > >-Original Message- > >From: Trahe, Fiona [mailto:fiona.tr...@intel.com] > >Sent: 12 March 2018 21:31 > >To: Ahmed Mansour ; Verma, Shally > >; > dev@dpdk.org > >Cc: De Lara Guarch, Pablo ; Athreya, > >Narayana Prasad > ; > >Gupta, Ashish ; Sahu, Sunila > >; Challa, Mahipal > >; Jain, Deepak K ; > >Hemant Agrawal > ; Roy > >Pledge ; Youri Querry ; > >fiona.tr...@gmail.com; Daly, > Lee ; > >Jozwiak, TomaszX > >Subject: RE: [dpdk-dev] [PATCH] compressdev: implement API - mbuf alternative > > > >Hi Shally, Ahmed, and anyone else interested in compressdev, > > > >I mentioned last week that we've been exploring using something other than > >mbufs to pass src/dst > buffers to compressdev PMDs. > > > >Reasons: > > - mbuf data is limited to 64k-1 in each segment of a chained mbuf. Data for > > compression > >can be greater and it would add cycles to have to break up into smaller > > segments. > > - data may originate in mbufs, but is more likely, particularly for storage > > use-cases, to > >originate in other data structures. > > - There's a 2 cache-line overhead for every segment in a chain, most of > > this data > >is network-related, not needed by compressdev > >So moving to a custom structure would minimise memory overhead, remove > >restriction on 64k-1 size and > give more flexibility if > >compressdev ever needs any comp-specific meta-data. > > > >We've come up with a compressdev-specific structure using the struct iovec > >from sys/uio.h, which is > commonly used by storage > >applications. This would replace the src and dest mbufs in the op. > >I'll not include the code here - Pablo will push that to github shortly and > >we'd appreciate review > comments there. > >https://github.com/pablodelara/dpdk-draft-compressdev > >Just posting on the mailing list to give a heads-up and ensure this reaches > >a wider audience than may see > it on github. > > > >Note : We also considered having no data structures in the op, instead the > >application > >would supply a callback which the PMD would use to retrieve meta-data (virt > >address, iova, length) > >for each next segment as needed. While this is quite flexible and allow the > >application > >to keep its data in its native structures, it's likely to cost more cycles. > >So we're not proposing this at the moment, but hope to benchmark it later > >while the API is still > experimental. > > > >General feedback on direction is welcome here on the mailing list. > >For feedback on the details of implementation we would appreciate comments > >on github. > > > >Regards, > >Fiona.
Re: [dpdk-dev] [PATCH] compressdev: implement API - mbuf alternative
Hi Fiona, > Hi Shally, Ahmed, and anyone else interested in compressdev, > > I mentioned last week that we've been exploring using something other than > mbufs to pass src/dst buffers to compressdev PMDs. > > Reasons: > - mbuf data is limited to 64k-1 in each segment of a chained mbuf. Data for > compression > can be greater and it would add cycles to have to break up into smaller > segments. > - data may originate in mbufs, but is more likely, particularly for storage > use-cases, to > originate in other data structures. > - There's a 2 cache-line overhead for every segment in a chain, most of this > data > is network-related, not needed by compressdev > So moving to a custom structure would minimise memory overhead, remove > restriction on 64k-1 size and give more flexibility if > compressdev ever needs any comp-specific meta-data. > > We've come up with a compressdev-specific structure using the struct iovec > from sys/uio.h, which is commonly used by storage > applications. This would replace the src and dest mbufs in the op. > I'll not include the code here - Pablo will push that to github shortly and > we'd appreciate review comments there. > https://github.com/pablodelara/dpdk-draft-compressdev > Just posting on the mailing list to give a heads-up and ensure this reaches a > wider audience than may see it on github. > > Note : We also considered having no data structures in the op, instead the > application > would supply a callback which the PMD would use to retrieve meta-data (virt > address, iova, length) > for each next segment as needed. While this is quite flexible and allow the > application > to keep its data in its native structures, it's likely to cost more cycles. As I said in different thread it will not only slowdown things, but will make it difficult (if possible at all) for compressdev PMDs to support DPDK MP model. Konstantin > So we're not proposing this at the moment, but hope to benchmark it later > while the API is still experimental. > > General feedback on direction is welcome here on the mailing list. > For feedback on the details of implementation we would appreciate comments on > github. > > Regards, > Fiona.
Re: [dpdk-dev] [PATCH] compressdev: implement API - mbuf alternative
HI Fiona So I understand we're moving away from mbufs because of its size limitation (64k) and cacheline overhead and their more suitability to n/w applications. Given that, I understand benefit of having another structure to input data but then what is proposal for ipcomp like application where mbuf usage may be a better option? Should we keep support for both (mbuf and this structure) so that apps can use appropriate data structure depending on their requirement. Further comments, on github. Thanks Shally >-Original Message- >From: Trahe, Fiona [mailto:fiona.tr...@intel.com] >Sent: 12 March 2018 21:31 >To: Ahmed Mansour ; Verma, Shally >; dev@dpdk.org >Cc: De Lara Guarch, Pablo ; Athreya, Narayana >Prasad ; >Gupta, Ashish ; Sahu, Sunila >; Challa, Mahipal >; Jain, Deepak K ; Hemant >Agrawal ; Roy >Pledge ; Youri Querry ; >fiona.tr...@gmail.com; Daly, Lee ; >Jozwiak, TomaszX >Subject: RE: [dpdk-dev] [PATCH] compressdev: implement API - mbuf alternative > >Hi Shally, Ahmed, and anyone else interested in compressdev, > >I mentioned last week that we've been exploring using something other than >mbufs to pass src/dst buffers to compressdev PMDs. > >Reasons: > - mbuf data is limited to 64k-1 in each segment of a chained mbuf. Data for > compression >can be greater and it would add cycles to have to break up into smaller > segments. > - data may originate in mbufs, but is more likely, particularly for storage > use-cases, to >originate in other data structures. > - There's a 2 cache-line overhead for every segment in a chain, most of this > data >is network-related, not needed by compressdev >So moving to a custom structure would minimise memory overhead, remove >restriction on 64k-1 size and give more flexibility if >compressdev ever needs any comp-specific meta-data. > >We've come up with a compressdev-specific structure using the struct iovec >from sys/uio.h, which is commonly used by storage >applications. This would replace the src and dest mbufs in the op. >I'll not include the code here - Pablo will push that to github shortly and >we'd appreciate review comments there. >https://github.com/pablodelara/dpdk-draft-compressdev >Just posting on the mailing list to give a heads-up and ensure this reaches a >wider audience than may see it on github. > >Note : We also considered having no data structures in the op, instead the >application >would supply a callback which the PMD would use to retrieve meta-data (virt >address, iova, length) >for each next segment as needed. While this is quite flexible and allow the >application >to keep its data in its native structures, it's likely to cost more cycles. >So we're not proposing this at the moment, but hope to benchmark it later >while the API is still experimental. > >General feedback on direction is welcome here on the mailing list. >For feedback on the details of implementation we would appreciate comments on >github. > >Regards, >Fiona.
Re: [dpdk-dev] [PATCH] compressdev: implement API - mbuf alternative
Hi Shally, Ahmed, and anyone else interested in compressdev, I mentioned last week that we've been exploring using something other than mbufs to pass src/dst buffers to compressdev PMDs. Reasons: - mbuf data is limited to 64k-1 in each segment of a chained mbuf. Data for compression can be greater and it would add cycles to have to break up into smaller segments. - data may originate in mbufs, but is more likely, particularly for storage use-cases, to originate in other data structures. - There's a 2 cache-line overhead for every segment in a chain, most of this data is network-related, not needed by compressdev So moving to a custom structure would minimise memory overhead, remove restriction on 64k-1 size and give more flexibility if compressdev ever needs any comp-specific meta-data. We've come up with a compressdev-specific structure using the struct iovec from sys/uio.h, which is commonly used by storage applications. This would replace the src and dest mbufs in the op. I'll not include the code here - Pablo will push that to github shortly and we'd appreciate review comments there. https://github.com/pablodelara/dpdk-draft-compressdev Just posting on the mailing list to give a heads-up and ensure this reaches a wider audience than may see it on github. Note : We also considered having no data structures in the op, instead the application would supply a callback which the PMD would use to retrieve meta-data (virt address, iova, length) for each next segment as needed. While this is quite flexible and allow the application to keep its data in its native structures, it's likely to cost more cycles. So we're not proposing this at the moment, but hope to benchmark it later while the API is still experimental. General feedback on direction is welcome here on the mailing list. For feedback on the details of implementation we would appreciate comments on github. Regards, Fiona.
Re: [dpdk-dev] [PATCH] compressdev: implement API
On 3/5/2018 9:32 AM, Verma, Shally wrote: > >> -Original Message- >> From: Ahmed Mansour [mailto:ahmed.mans...@nxp.com] >> Sent: 03 March 2018 01:19 >> To: Trahe, Fiona ; Verma, Shally >> ; dev@dpdk.org >> Cc: De Lara Guarch, Pablo ; Athreya, >> Narayana Prasad ; >> Gupta, Ashish ; Sahu, Sunila >> ; Challa, Mahipal >> ; Jain, Deepak K ; >> Hemant Agrawal ; Roy >> Pledge ; Youri Querry >> Subject: Re: [dpdk-dev] [PATCH] compressdev: implement API >> >> On 3/2/2018 4:53 AM, Trahe, Fiona wrote: >>>> On 3/1/2018 9:41 AM, Trahe, Fiona wrote: >>>>> Hi Shally >>>>> >>>>> //snip// >>>>>> [Shally] This looks better to me. So it mean app would always call >>>>>> xform_init() for stateless and attach >>>> an >>>>>> updated priv_xform to ops (depending upon if there's shareable or not). >>>>>> So it does not need to have >>>>>> NULL pointer on priv_xform. right? >>>>>> >>>>> [Fiona] yes. The PMD must return a valid priv_xform pointer. >>>> [Ahmed] What I understood is that the xform_init will be called once >>>> initially. if the @flag returned is NONE_SHAREABLE then the application >>>> must not attach two inflight ops to the same @priv_xform? Otherwise the >>>> application can attach many ops in flight to the @priv_xform? >>> [Fiona Yes. App calls the xform_init() once on a device where it plans to >>> send stateless ops. >>> If PMD returns shareable, then it doesn't need to call again and can attach >>> this to every stateless op going to that device. >>> If PMD returns SINGLE_OP then it must call xform_init() before every other >>> stateless op it wants to have inflight simultaneously. This does not mean >>> it must be called before every op, >>> but probably will set up a batch of priv_xforms - it can reuse each >>> priv_xform once the op finishes with it. >> [Ahmed] @Shally Can this complexity of managing the NONE_SHAREABLE mode >> be pushed into the PMD? A flexible stockpile can be kept and maintained >> by the PMD and it can be increased or decreased based on >> low-water/high-water thresholds > [Shally] It is doable to manage within PMD but need to do hands on to > evaluate effectiveness. So far, we have never exercised this way and left it > to application to attach different session (or stream) to op for maximum > performance gain. So, I would say, may it be ok to have flag feature in first > place and deprecate later, if it not required?! Or just have API without any > flag option and add a feature flag to indicate PMD support for > SHAREABLE/NON-SHAREABLE xform_priv handle?! [Ahmed] Either way looks ok to me. I see your point about performance. If this is in the PMD it will have to constantly guess how much memory the user needs and accommodate dynamically. The user can implement a similar scheme or if the application is simple they can pre-allocate and reduce CPU allocation de-allocation overhead.
Re: [dpdk-dev] [PATCH] compressdev: implement API
>-Original Message- >From: Ahmed Mansour [mailto:ahmed.mans...@nxp.com] >Sent: 03 March 2018 01:19 >To: Trahe, Fiona ; Verma, Shally >; dev@dpdk.org >Cc: De Lara Guarch, Pablo ; Athreya, Narayana >Prasad ; >Gupta, Ashish ; Sahu, Sunila >; Challa, Mahipal >; Jain, Deepak K ; Hemant >Agrawal ; Roy >Pledge ; Youri Querry >Subject: Re: [dpdk-dev] [PATCH] compressdev: implement API > >On 3/2/2018 4:53 AM, Trahe, Fiona wrote: >> >>> On 3/1/2018 9:41 AM, Trahe, Fiona wrote: >>>> Hi Shally >>>> >>>> //snip// >>>>> [Shally] This looks better to me. So it mean app would always call >>>>> xform_init() for stateless and attach >>> an >>>>> updated priv_xform to ops (depending upon if there's shareable or not). >>>>> So it does not need to have >>>>> NULL pointer on priv_xform. right? >>>>> >>>> [Fiona] yes. The PMD must return a valid priv_xform pointer. >>> [Ahmed] What I understood is that the xform_init will be called once >>> initially. if the @flag returned is NONE_SHAREABLE then the application >>> must not attach two inflight ops to the same @priv_xform? Otherwise the >>> application can attach many ops in flight to the @priv_xform? >> [Fiona Yes. App calls the xform_init() once on a device where it plans to >> send stateless ops. >> If PMD returns shareable, then it doesn't need to call again and can attach >> this to every stateless op going to that device. >> If PMD returns SINGLE_OP then it must call xform_init() before every other >> stateless op it wants to have inflight simultaneously. This does not mean it >> must be called before every op, >> but probably will set up a batch of priv_xforms - it can reuse each >> priv_xform once the op finishes with it. >[Ahmed] @Shally Can this complexity of managing the NONE_SHAREABLE mode >be pushed into the PMD? A flexible stockpile can be kept and maintained >by the PMD and it can be increased or decreased based on >low-water/high-water thresholds [Shally] It is doable to manage within PMD but need to do hands on to evaluate effectiveness. So far, we have never exercised this way and left it to application to attach different session (or stream) to op for maximum performance gain. So, I would say, may it be ok to have flag feature in first place and deprecate later, if it not required?! Or just have API without any flag option and add a feature flag to indicate PMD support for SHAREABLE/NON-SHAREABLE xform_priv handle?!
Re: [dpdk-dev] [PATCH] compressdev: implement API
On 2/28/2018 1:39 PM, Trahe, Fiona wrote:
> Hi Ahmed, Shally,
>
> So just to capture what we concluded in the call today:
>
> - There's no requirement for a device-agnostic session to facilitate
> load-balancing.
> - For stateful data a stream is compulsory. Xform is passed to stream on
> creation.
> So no need for a session in stateful op.
>
> Re session data for stateless ops:
> - All PMDs could cope with just passing in a xform with a stateless op. But
> it might
>not be performant.
> - Some PMDs need to allocate some resources which can only be used by one op
>at a time. For stateful ops these resources can be attached to a stream.
> For stateless
>they could allocate the resources on each op enqueue, but it would be
> better if
>the resources were setup once based on the xform and could be re-used on
> ops,
>though only by one op at a time.
> - Some PMDs don't need to allocate such resources, but could benefit by
>setting up some pmd data based on the xform. This data would not be
>constrained, could be used in parallel by any op or qp of the device.
> - The name pmd_stateless_data was not popular, maybe something like
> xform_private_data can be used. On creation of this data, the PMD can
> return
> an indication of whether it should be used by one op at a time or shared.
>
>
> So I'll
> - remove the session completely from the API.
> - add an initialiser API for the data to be attached to stateless ops
> - add a union to the op:
>
> union {
> void *pmd_private_xform;
> /**< Stateless private PMD data derived from an rte_comp_xform
> * rte_comp_xform_init() must be called on a device
> * before sending any STATELESS operations. The PMD returns a handle
> * which must be attached to subsequent STATELESS operations.
> * The PMD also returns a flag, if this is
> COMP_PRIVATE_XFORM_SHAREABLE
> * then the xform can be attached to multiple ops at the same time,
> * if it's COMP_PRIVATE_XFORM_SINGLE_OP then it can only be
> * be used on one op at a time, other private xforms must be
> initialised
> * to send other ops in parallel.
> */
> void *stream;
> /* Private PMD data derived initially from an rte_comp_xform, which
> holds state
> * and history data and evolves as operations are processed.
> * rte_comp_stream_create() must be called on a device for all
> STATEFUL
> * data streams and the resulting stream attached
> * to the one or more operations associated with the data stream.
> * All operations in a stream must be sent to the same device.
> */
> }
>
> Previous startup flow before sending a stateful op:
> rte_comp_get_private_size(devid)
> rte_comp_mempool_create() - returns sess_pool
> rte_comp_session_create(sess_pool)
> rte_comp_session_init(devid, sess, sess_pool, xform)
> rte_comp_stream_create(devid, sess, **stream, op_type)
>
> simplified to:
> rte_comp_xform_init(devid, xform, **priv_xform, *flag) - returns handle and
> flag
> (pool is within the PMD)
>
> Note, I don't think we bottomed out on removing the xform from the union, but
> I don't
> think we need it with above solution.
>
> Other discussion:
> - we should document on API that qp is not thread-safe, so enqueue
>and dequeue should be performed by same thread.
[Ahmed] - I understand a qp should represent a single software user.
This is good because we will not have to add locking as you mentioned,
but are you sure that dequeues cannot be performed by another thread
without adding significant overhead? it would enable producer consumer
applications, and the name queue pair implies some independence.
- Another question. I want to be sure. A qp can be used to send both
compress and decompress ops.
- Nitpick: The name queue pair implies order preservation to the user.
Maybe we should change it to something that does not imply that.
>
> device and qp flow:
> - dev_info_get() - application reads device capabilities, including the max
> qps the device can support.
> - dev_config() - application specifies how many qps it intends to use -
> typically one per thread, must be < device max
> - qp_setup() - called per qp. Creates the qp based on the size indicated by
> max_inflights
> - dev_start() - once started device can't be reconfigured, must call
> dev_stop to reconfigure.
>
>
> Regards,
> Fiona
>
>> -Original Message-
>> From: Verma, Shally [mailto:shally.ve...@cavium.com]
>> Sent: Tuesday, February 27, 2018 5:54 AM
>
Re: [dpdk-dev] [PATCH] compressdev: implement API
On 3/2/2018 4:53 AM, Trahe, Fiona wrote: > >> On 3/1/2018 9:41 AM, Trahe, Fiona wrote: >>> Hi Shally >>> >>> //snip// [Shally] This looks better to me. So it mean app would always call xform_init() for stateless and attach >> an updated priv_xform to ops (depending upon if there's shareable or not). So it does not need to have NULL pointer on priv_xform. right? >>> [Fiona] yes. The PMD must return a valid priv_xform pointer. >> [Ahmed] What I understood is that the xform_init will be called once >> initially. if the @flag returned is NONE_SHAREABLE then the application >> must not attach two inflight ops to the same @priv_xform? Otherwise the >> application can attach many ops in flight to the @priv_xform? > [Fiona Yes. App calls the xform_init() once on a device where it plans to > send stateless ops. > If PMD returns shareable, then it doesn't need to call again and can attach > this to every stateless op going to that device. > If PMD returns SINGLE_OP then it must call xform_init() before every other > stateless op it wants to have inflight simultaneously. This does not mean it > must be called before every op, > but probably will set up a batch of priv_xforms - it can reuse each > priv_xform once the op finishes with it. [Ahmed] @Shally Can this complexity of managing the NONE_SHAREABLE mode be pushed into the PMD? A flexible stockpile can be kept and maintained by the PMD and it can be increased or decreased based on low-water/high-water thresholds
Re: [dpdk-dev] [PATCH] compressdev: implement API
> -Original Message- > From: Ahmed Mansour [mailto:ahmed.mans...@nxp.com] > Sent: Friday, March 2, 2018 12:56 AM > To: Trahe, Fiona ; Verma, Shally > ; dev@dpdk.org > Cc: De Lara Guarch, Pablo ; Athreya, Narayana > Prasad > ; Gupta, Ashish ; > Sahu, Sunila > ; Challa, Mahipal ; Jain, > Deepak K > ; Hemant Agrawal ; Roy Pledge > ; Youri Querry > Subject: Re: [dpdk-dev] [PATCH] compressdev: implement API > > On 3/1/2018 9:41 AM, Trahe, Fiona wrote: > > Hi Shally > > > > //snip// > >> [Shally] This looks better to me. So it mean app would always call > >> xform_init() for stateless and attach > an > >> updated priv_xform to ops (depending upon if there's shareable or not). So > >> it does not need to have > >> NULL pointer on priv_xform. right? > >> > > [Fiona] yes. The PMD must return a valid priv_xform pointer. > > [Ahmed] What I understood is that the xform_init will be called once > initially. if the @flag returned is NONE_SHAREABLE then the application > must not attach two inflight ops to the same @priv_xform? Otherwise the > application can attach many ops in flight to the @priv_xform? [Fiona Yes. App calls the xform_init() once on a device where it plans to send stateless ops. If PMD returns shareable, then it doesn't need to call again and can attach this to every stateless op going to that device. If PMD returns SINGLE_OP then it must call xform_init() before every other stateless op it wants to have inflight simultaneously. This does not mean it must be called before every op, but probably will set up a batch of priv_xforms - it can reuse each priv_xform once the op finishes with it.
Re: [dpdk-dev] [PATCH] compressdev: implement API
On 3/1/2018 9:41 AM, Trahe, Fiona wrote: > Hi Shally > > //snip// >> [Shally] This looks better to me. So it mean app would always call >> xform_init() for stateless and attach an >> updated priv_xform to ops (depending upon if there's shareable or not). So >> it does not need to have >> NULL pointer on priv_xform. right? >> > [Fiona] yes. The PMD must return a valid priv_xform pointer. [Ahmed] What I understood is that the xform_init will be called once initially. if the @flag returned is NONE_SHAREABLE then the application must not attach two inflight ops to the same @priv_xform? Otherwise the application can attach many ops in flight to the @priv_xform?
Re: [dpdk-dev] [PATCH] compressdev: implement API
Hi Shally //snip// > [Shally] This looks better to me. So it mean app would always call > xform_init() for stateless and attach an > updated priv_xform to ops (depending upon if there's shareable or not). So it > does not need to have > NULL pointer on priv_xform. right? > [Fiona] yes. The PMD must return a valid priv_xform pointer.
Re: [dpdk-dev] [PATCH] compressdev: implement API
Hi Fiona
>-Original Message-
>From: Trahe, Fiona [mailto:fiona.tr...@intel.com]
>Sent: 01 March 2018 00:09
>To: Verma, Shally ; Ahmed Mansour
>; dev@dpdk.org
>Cc: De Lara Guarch, Pablo ; Athreya, Narayana
>Prasad ;
>Gupta, Ashish ; Sahu, Sunila
>; Challa, Mahipal
>; Jain, Deepak K ; Hemant
>Agrawal ; Roy
>Pledge ; Youri Querry ; Trahe,
>Fiona
>Subject: RE: [dpdk-dev] [PATCH] compressdev: implement API
>
>Hi Ahmed, Shally,
>
>So just to capture what we concluded in the call today:
>
> - There's no requirement for a device-agnostic session to facilitate
> load-balancing.
> - For stateful data a stream is compulsory. Xform is passed to stream on
> creation.
>So no need for a session in stateful op.
>
>Re session data for stateless ops:
> - All PMDs could cope with just passing in a xform with a stateless op. But
> it might
> not be performant.
> - Some PMDs need to allocate some resources which can only be used by one op
> at a time. For stateful ops these resources can be attached to a stream.
> For stateless
> they could allocate the resources on each op enqueue, but it would be
> better if
> the resources were setup once based on the xform and could be re-used on
> ops,
> though only by one op at a time.
> - Some PMDs don't need to allocate such resources, but could benefit by
> setting up some pmd data based on the xform. This data would not be
> constrained, could be used in parallel by any op or qp of the device.
> - The name pmd_stateless_data was not popular, maybe something like
>xform_private_data can be used. On creation of this data, the PMD can
> return
>an indication of whether it should be used by one op at a time or shared.
>
>So I'll
> - remove the session completely from the API.
> - add an initialiser API for the data to be attached to stateless ops
> - add a union to the op:
>
> union {
>void *pmd_private_xform;
>/**< Stateless private PMD data derived from an rte_comp_xform
> * rte_comp_xform_init() must be called on a device
> * before sending any STATELESS operations. The PMD returns a handle
> * which must be attached to subsequent STATELESS operations.
> * The PMD also returns a flag, if this is COMP_PRIVATE_XFORM_SHAREABLE
> * then the xform can be attached to multiple ops at the same time,
> * if it's COMP_PRIVATE_XFORM_SINGLE_OP then it can only be
> * be used on one op at a time, other private xforms must be
> initialised
> * to send other ops in parallel.
> */
>void *stream;
>/* Private PMD data derived initially from an rte_comp_xform, which
> holds state
> * and history data and evolves as operations are processed.
> * rte_comp_stream_create() must be called on a device for all STATEFUL
> * data streams and the resulting stream attached
> * to the one or more operations associated with the data stream.
> * All operations in a stream must be sent to the same device.
> */
>}
>
>Previous startup flow before sending a stateful op:
>rte_comp_get_private_size(devid)
>rte_comp_mempool_create() - returns sess_pool
>rte_comp_session_create(sess_pool)
>rte_comp_session_init(devid, sess, sess_pool, xform)
>rte_comp_stream_create(devid, sess, **stream, op_type)
>
>simplified to:
>rte_comp_xform_init(devid, xform, **priv_xform, *flag) - returns handle and
>flag
>(pool is within the PMD)
>
>Note, I don't think we bottomed out on removing the xform from the union, but
>I don't
>think we need it with above solution.
[Shally] This looks better to me. So it mean app would always call xform_init()
for stateless and attach an updated priv_xform to ops (depending upon if
there's shareable or not). So it does not need to have NULL pointer on
priv_xform. right?
>
>Other discussion:
> - we should document on API that qp is not thread-safe, so enqueue
> and dequeue should be performed by same thread.
>
>device and qp flow:
> - dev_info_get() - application reads device capabilities, including the max
> qps the device can support.
> - dev_config() - application specifies how many qps it intends to use -
> typically one per thread, must be < device max
> - qp_setup() - called per qp. Creates the qp based on the size indicated by
> max_inflights
> - dev_start() - once started device can't be reconfigured, must call dev_stop
> to reconfigure.
>
>
>Regards,
>Fiona
>
>> -Original Message-
>> From: Verma, Shally [mailto:shally.ve...@cavium.com]
>> Sent: Tuesday, February 27, 2018 5:54 AM
>> T
Re: [dpdk-dev] [PATCH] compressdev: implement API
Hi Ahmed, Shally,
So just to capture what we concluded in the call today:
- There's no requirement for a device-agnostic session to facilitate
load-balancing.
- For stateful data a stream is compulsory. Xform is passed to stream on
creation.
So no need for a session in stateful op.
Re session data for stateless ops:
- All PMDs could cope with just passing in a xform with a stateless op. But it
might
not be performant.
- Some PMDs need to allocate some resources which can only be used by one op
at a time. For stateful ops these resources can be attached to a stream. For
stateless
they could allocate the resources on each op enqueue, but it would be better
if
the resources were setup once based on the xform and could be re-used on ops,
though only by one op at a time.
- Some PMDs don't need to allocate such resources, but could benefit by
setting up some pmd data based on the xform. This data would not be
constrained, could be used in parallel by any op or qp of the device.
- The name pmd_stateless_data was not popular, maybe something like
xform_private_data can be used. On creation of this data, the PMD can
return
an indication of whether it should be used by one op at a time or shared.
So I'll
- remove the session completely from the API.
- add an initialiser API for the data to be attached to stateless ops
- add a union to the op:
union {
void *pmd_private_xform;
/**< Stateless private PMD data derived from an rte_comp_xform
* rte_comp_xform_init() must be called on a device
* before sending any STATELESS operations. The PMD returns a handle
* which must be attached to subsequent STATELESS operations.
* The PMD also returns a flag, if this is COMP_PRIVATE_XFORM_SHAREABLE
* then the xform can be attached to multiple ops at the same time,
* if it's COMP_PRIVATE_XFORM_SINGLE_OP then it can only be
* be used on one op at a time, other private xforms must be initialised
* to send other ops in parallel.
*/
void *stream;
/* Private PMD data derived initially from an rte_comp_xform, which
holds state
* and history data and evolves as operations are processed.
* rte_comp_stream_create() must be called on a device for all STATEFUL
* data streams and the resulting stream attached
* to the one or more operations associated with the data stream.
* All operations in a stream must be sent to the same device.
*/
}
Previous startup flow before sending a stateful op:
rte_comp_get_private_size(devid)
rte_comp_mempool_create() - returns sess_pool
rte_comp_session_create(sess_pool)
rte_comp_session_init(devid, sess, sess_pool, xform)
rte_comp_stream_create(devid, sess, **stream, op_type)
simplified to:
rte_comp_xform_init(devid, xform, **priv_xform, *flag) - returns handle and
flag
(pool is within the PMD)
Note, I don't think we bottomed out on removing the xform from the union, but I
don't
think we need it with above solution.
Other discussion:
- we should document on API that qp is not thread-safe, so enqueue
and dequeue should be performed by same thread.
device and qp flow:
- dev_info_get() - application reads device capabilities, including the max
qps the device can support.
- dev_config() - application specifies how many qps it intends to use -
typically one per thread, must be < device max
- qp_setup() - called per qp. Creates the qp based on the size indicated by
max_inflights
- dev_start() - once started device can't be reconfigured, must call dev_stop
to reconfigure.
Regards,
Fiona
> -Original Message-
> From: Verma, Shally [mailto:shally.ve...@cavium.com]
> Sent: Tuesday, February 27, 2018 5:54 AM
> To: Ahmed Mansour ; Trahe, Fiona
> ;
> dev@dpdk.org
> Cc: De Lara Guarch, Pablo ; Athreya, Narayana
> Prasad
> ; Gupta, Ashish ;
> Sahu, Sunila
> ; Challa, Mahipal ; Jain,
> Deepak K
> ; Hemant Agrawal ; Roy Pledge
> ; Youri Querry
> Subject: RE: [dpdk-dev] [PATCH] compressdev: implement API
>
>
>
> >-Original Message-
> >From: Ahmed Mansour [mailto:ahmed.mans...@nxp.com]
> >Sent: 27 February 2018 03:05
> >To: Verma, Shally ; Trahe, Fiona
> >; dev@dpdk.org
> >Cc: De Lara Guarch, Pablo ; Athreya,
> >Narayana Prasad
> ;
> >Gupta, Ashish ; Sahu, Sunila
> >; Challa,
> Mahipal
> >; Jain, Deepak K ;
> >Hemant Agrawal
> ; Roy
> >Pledge ; Youri Querry
> >Subject: Re: [dpdk-dev] [PATCH] compressdev: implement API
> >
> >> Hi Fiona, Ahmed
> >>> Hi Fiona,
> >>>
> >>> Thanks for starting this discussion. In the current API the user must
> >>> make 12 API calls just to get information to
Re: [dpdk-dev] [PATCH] compressdev: implement API
>-Original Message- >From: Ahmed Mansour [mailto:ahmed.mans...@nxp.com] >Sent: 27 February 2018 03:05 >To: Verma, Shally ; Trahe, Fiona >; dev@dpdk.org >Cc: De Lara Guarch, Pablo ; Athreya, Narayana >Prasad ; >Gupta, Ashish ; Sahu, Sunila >; Challa, Mahipal >; Jain, Deepak K ; Hemant >Agrawal ; Roy >Pledge ; Youri Querry >Subject: Re: [dpdk-dev] [PATCH] compressdev: implement API > >> Hi Fiona, Ahmed >>> Hi Fiona, >>> >>> Thanks for starting this discussion. In the current API the user must >>> make 12 API calls just to get information to compress. Maybe there is a >>> way to simplify. At least for some use cases (stateless). I think a call >>> sometime next week would be good to help clarify coalesce some of the >>> complexity. >>> >>> I added specific comments inline. >>> >>> Thanks, >>> >>> Ahmed >>> >>> On 2/21/2018 2:12 PM, Trahe, Fiona wrote: >>>> We've been struggling with the idea of session in compressdev. >>>> >>>> Is it really a session? >>>> - It's not in the same sense as cryptodev where it's used to hold a key, >>>> and maps to a Security Association. >>>> - It's a set of immutable data that is needed with the op and stream to >>>> perform the operation. >>>> - It inherited from cryptodev the ability to be set up for multiple >>>> driver types and used across any >>>> devices of those types. For stateful ops this facility can't be used. >>>> For stateless we don't think it's important, and think it's unlikely >>>> to be used. >>>> - Drivers use it to prepare private data, set up resources, do pre-work, >>>> so there's >>>> less work to be done on the data path. Initially we didn't have a >>>> stream, we do now, >>>> this may be a better alternative place for that work. >>>> So we've been toying with the idea of getting rid of the session. >>> [Ahmed] In our proprietary API the stream and session are one. A session >>> holds many properties like the op-type, instead of having this >>> information in the op itself. This way we lower the per op setup cost. >>> This also allows rapid reuse of stateful infrastructure, once a stream >>> is closed on a stateful session, the next op (stream) on this session >>> reuses the stateful storage. Obviously if a stream is in "pause mode" on >>> a session, all following ops that may be unrelated to this >>> stream/session must also wait until this current stream is closed or >>> aborted before the infrastructure can be reused. >>>> We also struggle with the idea of setting up a stream for stateless ops. >>>> - Well, really I just think the name is misleading, i.e. there's no >>>> problem with setting >>>> up some private PMD data to use with stateless operations, just >>>> calling it a >>>> stream doesn't seem right. >>> [Ahmed] I agree. The op has all the necessary information to process it >>> in the current API? Both the stream and the op are one time use. We >>> can't attach multiple similar ops to a single stream/session and rely on >>> their properties to simplify op setup, so why the hassle. >> [Shally] As per my knowledge, session came with idea in DPDK, if system has >> multiple devices setup to do similar jobs then >application can fan out ops to any of them for load-balancing. Though it is >not possible for stateful ops but it still can work for stateless. >If there's an application which only have stateless ops to process then I see >this is still useful feature to support. >[Ahmed] Is there an advantage to exposing load balancing to the user? I >do not see load balancing as a feature within itself. Can the PMD take >care of this? I guess a system that has [Shally] I assume idea was to leverage multiple PMDs that are available in system (say QAT+SW ZLIB) and I believe matter of load-balancing came out of one of the earlier discussion with Fiona on RFC v1. http://dev.dpdk.narkive.com/CHS5l01B/dpdk-dev-rfc-v1-doc-compression-api-for-dpdk#post3 So, I wait for her comments on this. But in any case, with changed notion too it looks achievable to me, if so is desired. >> In current proposal, stream logically represent data and hold its specific >> information and session is generic information that can be >applied on multiple data. If we want to combine stream and session. Then o
Re: [dpdk-dev] [PATCH] compressdev: implement API
>-Original Message- >From: Ahmed Mansour [mailto:ahmed.mans...@nxp.com] >Sent: 27 February 2018 00:46 >To: Trahe, Fiona ; dev@dpdk.org; Verma, Shally > >Cc: De Lara Guarch, Pablo ; Athreya, Narayana >Prasad ; >Gupta, Ashish ; Sahu, Sunila >; Challa, Mahipal >; Jain, Deepak K ; Hemant >Agrawal ; Roy >Pledge ; Youri Querry >Subject: Re: [dpdk-dev] [PATCH] compressdev: implement API > >>> Hi Fiona, >>> >>> Thanks for starting this discussion. In the current API the user must >>> make 12 API calls just to get information to compress. Maybe there is a >>> way to simplify. At least for some use cases (stateless). I think a call >>> sometime next week would be good to help clarify coalesce some of the >>> complexity. >> [Fiona] Would 10:30 GMT on Wednesday 28th Feb suit? >[Ahmed] I am in Ottawa Canada so Wednesday 12:00 or 1:00 GMT would be >better. Does that work? [Shally] Am in India time zone. Either works for me. //snip
Re: [dpdk-dev] [PATCH] compressdev: implement API
> Hi Fiona, Ahmed >> Hi Fiona, >> >> Thanks for starting this discussion. In the current API the user must >> make 12 API calls just to get information to compress. Maybe there is a >> way to simplify. At least for some use cases (stateless). I think a call >> sometime next week would be good to help clarify coalesce some of the >> complexity. >> >> I added specific comments inline. >> >> Thanks, >> >> Ahmed >> >> On 2/21/2018 2:12 PM, Trahe, Fiona wrote: >>> We've been struggling with the idea of session in compressdev. >>> >>> Is it really a session? >>> - It's not in the same sense as cryptodev where it's used to hold a key, >>> and maps to a Security Association. >>> - It's a set of immutable data that is needed with the op and stream to >>> perform the operation. >>> - It inherited from cryptodev the ability to be set up for multiple driver >>> types and used across any >>> devices of those types. For stateful ops this facility can't be used. >>> For stateless we don't think it's important, and think it's unlikely to >>> be used. >>> - Drivers use it to prepare private data, set up resources, do pre-work, >>> so there's >>> less work to be done on the data path. Initially we didn't have a >>> stream, we do now, >>> this may be a better alternative place for that work. >>> So we've been toying with the idea of getting rid of the session. >> [Ahmed] In our proprietary API the stream and session are one. A session >> holds many properties like the op-type, instead of having this >> information in the op itself. This way we lower the per op setup cost. >> This also allows rapid reuse of stateful infrastructure, once a stream >> is closed on a stateful session, the next op (stream) on this session >> reuses the stateful storage. Obviously if a stream is in "pause mode" on >> a session, all following ops that may be unrelated to this >> stream/session must also wait until this current stream is closed or >> aborted before the infrastructure can be reused. >>> We also struggle with the idea of setting up a stream for stateless ops. >>> - Well, really I just think the name is misleading, i.e. there's no >>> problem with setting >>> up some private PMD data to use with stateless operations, just calling >>> it a >>> stream doesn't seem right. >> [Ahmed] I agree. The op has all the necessary information to process it >> in the current API? Both the stream and the op are one time use. We >> can't attach multiple similar ops to a single stream/session and rely on >> their properties to simplify op setup, so why the hassle. > [Shally] As per my knowledge, session came with idea in DPDK, if system has > multiple devices setup to do similar jobs then application can fan out ops to > any of them for load-balancing. Though it is not possible for stateful ops > but it still can work for stateless. If there's an application which only > have stateless ops to process then I see this is still useful feature to > support. [Ahmed] Is there an advantage to exposing load balancing to the user? I do not see load balancing as a feature within itself. Can the PMD take care of this? I guess a system that has > In current proposal, stream logically represent data and hold its specific > information and session is generic information that can be applied on > multiple data. If we want to combine stream and session. Then one way to look > at this is: > > "let application only allocate and initialize session with rte_comp_xform > (and possibly op type) information so that PMD can do one-time setup and > allocate enough resources. Once attached to op, cannot be reused until that > op is fully processed. So, if app has 16 data elements to process in a burst, > it will setup 16 sessions." [Ahmed] Why not allow multiple inflight stateless ops with the same session? Stateless by definition guarantees that the resources used to work on one up will be free after the op is processed. That means that even if an op fails to process correctly on a session, it will have no effect on the next op since there is not interdependence. This assumes that the resources are shareable between hardware instances for stateless. That is not a bad assumption since hardware should not need more than the data of the op itself to work on a statelss op. > This is same as what Ahmed suggested. For a particular load-balancing case > suggested above, If application want, can initialize different sessions on > multiple devices with same xform so that each is prepared to process ops. > Application can then fanout stateless ops to multiple devices for > load-balancing but then it would need to keep map of device & a session map. > > If this sound feasible, then I too believe we can rather get rid of either > and keep one (possibly session but am open with stream as well). > However, regardless of case whether we live with name stream or session, I > don't see much deviation from current API spec except descript
Re: [dpdk-dev] [PATCH] compressdev: implement API
>> Hi Fiona,
>>
>> Thanks for starting this discussion. In the current API the user must
>> make 12 API calls just to get information to compress. Maybe there is a
>> way to simplify. At least for some use cases (stateless). I think a call
>> sometime next week would be good to help clarify coalesce some of the
>> complexity.
> [Fiona] Would 10:30 GMT on Wednesday 28th Feb suit?
[Ahmed] I am in Ottawa Canada so Wednesday 12:00 or 1:00 GMT would be
better. Does that work?
>> I added specific comments inline.
>>
>> Thanks,
>>
>> Ahmed
>>
>> On 2/21/2018 2:12 PM, Trahe, Fiona wrote:
>>> We've been struggling with the idea of session in compressdev.
>>>
>>> Is it really a session?
>>> - It's not in the same sense as cryptodev where it's used to hold a key,
>>> and maps to a Security
>> Association.
>>> - It's a set of immutable data that is needed with the op and stream to
>>> perform the operation.
>>> - It inherited from cryptodev the ability to be set up for multiple driver
>>> types and used across any
>>> devices of those types. For stateful ops this facility can't be used.
>>> For stateless we don't think it's important, and think it's unlikely to
>>> be used.
>>> - Drivers use it to prepare private data, set up resources, do pre-work,
>>> so there's
>>> less work to be done on the data path. Initially we didn't have a
>>> stream, we do now,
>>> this may be a better alternative place for that work.
>>> So we've been toying with the idea of getting rid of the session.
>> [Ahmed] In our proprietary API the stream and session are one. A session
>> holds many properties like the op-type, instead of having this
>> information in the op itself. This way we lower the per op setup cost.
>> This also allows rapid reuse of stateful infrastructure, once a stream
>> is closed on a stateful session, the next op (stream) on this session
>> reuses the stateful storage. Obviously if a stream is in "pause mode" on
>> a session, all following ops that may be unrelated to this
>> stream/session must also wait until this current stream is closed or
>> aborted before the infrastructure can be reused.
>>> We also struggle with the idea of setting up a stream for stateless ops.
>>> - Well, really I just think the name is misleading, i.e. there's no
>>> problem with setting
>>> up some private PMD data to use with stateless operations, just calling
>>> it a
>>> stream doesn't seem right.
>> [Ahmed] I agree. The op has all the necessary information to process it
>> in the current API? Both the stream and the op are one time use. We
>> can't attach multiple similar ops to a single stream/session and rely on
>> their properties to simplify op setup, so why the hassle.
>>> So putting above thoughts together I want to propose:
>>> - Removal of the session and all associated APIs.
>>> - Passing in one of three data types in the rte_comp_op
>>>
>>> union {
>>> struct rte_comp_xform *xform;
>>> /**< Immutable compress/decompress params */
>>> void *pmd_stateless_data;
>>> /**< Stateless private PMD data derived from an rte_comp_xform
>>> * rte_comp_stateless_data_init() must be called on a device
>>> * before sending any STATELESS operations. If the PMD returns a
>>> non-NULL
>>> * value the handle must be attached to subsequent STATELESS
>>> operations.
>>> * If a PMD returns NULL, then the xform should be passed directly
>>> to each op
>>> */
>>> void *stream;
>>> /* Private PMD data derived initially from an rte_comp_xform, which
>>> holds state
>>> * and history data and evolves as operations are processed.
>>> * rte_comp_stream_create() must be called on a device for all
>>> STATEFUL
>>> * data streams and the resulting stream attached
>>> * to the one or more operations associated with the data stream.
>>> * All operations in a stream must be sent to the same device.
>>> */
>>> }
>> [Ahmed] I like this setup, but I am not sure in what cases the xform
>> immutable would be used. I understand the other two.
> [Fiona] The xform is there because I don't know yet what limitations may
> apply to the
> pmd_stateless_data. If it has no limitation and once set up once on a device
> can be attached simultaneously to any op sent to any qp on that device
> then we don't need the xform. But I understood from Shally's earlier request
> for
> setting up a stream on a stateless request that some resources are
> allocated, so we may need to document some limitations.
> In this case the xform may be a better path for PMDs which don't have the same
> limitations.
>
>
>>> Notes:
>>> 1. Internally if a PMD wants to use the exact same data structure for both
>>> it can do,
>>> just on the API I think it's better if they're named differently with
>>> different comments.
>>> 2. I'm not clear of the constraints if any, which attach to the
Re: [dpdk-dev] [PATCH] compressdev: implement API
Hi Fiona, Ahmed >-Original Message- >From: Ahmed Mansour [mailto:ahmed.mans...@nxp.com] >Sent: 24 February 2018 06:47 >To: Trahe, Fiona ; dev@dpdk.org; Verma, Shally > >Cc: De Lara Guarch, Pablo ; Athreya, Narayana >Prasad ; >Gupta, Ashish ; Sahu, Sunila >; Challa, Mahipal >; Jain, Deepak K ; Hemant >Agrawal ; Roy >Pledge ; Youri Querry >Subject: Re: [dpdk-dev] [PATCH] compressdev: implement API > >Hi Fiona, > >Thanks for starting this discussion. In the current API the user must >make 12 API calls just to get information to compress. Maybe there is a >way to simplify. At least for some use cases (stateless). I think a call >sometime next week would be good to help clarify coalesce some of the >complexity. > >I added specific comments inline. > >Thanks, > >Ahmed > >On 2/21/2018 2:12 PM, Trahe, Fiona wrote: >> We've been struggling with the idea of session in compressdev. >> >> Is it really a session? >> - It's not in the same sense as cryptodev where it's used to hold a key, >> and maps to a Security Association. >> - It's a set of immutable data that is needed with the op and stream to >> perform the operation. >> - It inherited from cryptodev the ability to be set up for multiple driver >> types and used across any >> devices of those types. For stateful ops this facility can't be used. >> For stateless we don't think it's important, and think it's unlikely to >> be used. >> - Drivers use it to prepare private data, set up resources, do pre-work, so >> there's >> less work to be done on the data path. Initially we didn't have a >> stream, we do now, >> this may be a better alternative place for that work. >> So we've been toying with the idea of getting rid of the session. >[Ahmed] In our proprietary API the stream and session are one. A session >holds many properties like the op-type, instead of having this >information in the op itself. This way we lower the per op setup cost. >This also allows rapid reuse of stateful infrastructure, once a stream >is closed on a stateful session, the next op (stream) on this session >reuses the stateful storage. Obviously if a stream is in "pause mode" on >a session, all following ops that may be unrelated to this >stream/session must also wait until this current stream is closed or >aborted before the infrastructure can be reused. >> >> We also struggle with the idea of setting up a stream for stateless ops. >> - Well, really I just think the name is misleading, i.e. there's no >> problem with setting >> up some private PMD data to use with stateless operations, just calling >> it a >> stream doesn't seem right. >[Ahmed] I agree. The op has all the necessary information to process it >in the current API? Both the stream and the op are one time use. We >can't attach multiple similar ops to a single stream/session and rely on >their properties to simplify op setup, so why the hassle. [Shally] As per my knowledge, session came with idea in DPDK, if system has multiple devices setup to do similar jobs then application can fan out ops to any of them for load-balancing. Though it is not possible for stateful ops but it still can work for stateless. If there's an application which only have stateless ops to process then I see this is still useful feature to support. In current proposal, stream logically represent data and hold its specific information and session is generic information that can be applied on multiple data. If we want to combine stream and session. Then one way to look at this is: "let application only allocate and initialize session with rte_comp_xform (and possibly op type) information so that PMD can do one-time setup and allocate enough resources. Once attached to op, cannot be reused until that op is fully processed. So, if app has 16 data elements to process in a burst, it will setup 16 sessions." This is same as what Ahmed suggested. For a particular load-balancing case suggested above, If application want, can initialize different sessions on multiple devices with same xform so that each is prepared to process ops. Application can then fanout stateless ops to multiple devices for load-balancing but then it would need to keep map of device & a session map. If this sound feasible, then I too believe we can rather get rid of either and keep one (possibly session but am open with stream as well). However, regardless of case whether we live with name stream or session, I don't see much deviation from current API spec except description and few modifications/additions as identified. So, then I see it as:
Re: [dpdk-dev] [PATCH] compressdev: implement API
> -Original Message-
> From: Ahmed Mansour [mailto:ahmed.mans...@nxp.com]
> Sent: Saturday, February 24, 2018 1:17 AM
> To: Trahe, Fiona ; dev@dpdk.org;
> shally.ve...@cavium.com
> Cc: De Lara Guarch, Pablo ; Athreya, Narayana
> Prasad
> ; Gupta, Ashish ;
> Sahu, Sunila
> ; Challa, Mahipal ; Jain,
> Deepak K
> ; Hemant Agrawal ; Roy Pledge
> ; Youri Querry
> Subject: Re: [dpdk-dev] [PATCH] compressdev: implement API
>
> Hi Fiona,
>
> Thanks for starting this discussion. In the current API the user must
> make 12 API calls just to get information to compress. Maybe there is a
> way to simplify. At least for some use cases (stateless). I think a call
> sometime next week would be good to help clarify coalesce some of the
> complexity.
[Fiona] Would 10:30 GMT on Wednesday 28th Feb suit?
>
> I added specific comments inline.
>
> Thanks,
>
> Ahmed
>
> On 2/21/2018 2:12 PM, Trahe, Fiona wrote:
> > We've been struggling with the idea of session in compressdev.
> >
> > Is it really a session?
> > - It's not in the same sense as cryptodev where it's used to hold a key,
> > and maps to a Security
> Association.
> > - It's a set of immutable data that is needed with the op and stream to
> > perform the operation.
> > - It inherited from cryptodev the ability to be set up for multiple driver
> > types and used across any
> > devices of those types. For stateful ops this facility can't be used.
> > For stateless we don't think it's important, and think it's unlikely to
> > be used.
> > - Drivers use it to prepare private data, set up resources, do pre-work,
> > so there's
> > less work to be done on the data path. Initially we didn't have a
> > stream, we do now,
> > this may be a better alternative place for that work.
> > So we've been toying with the idea of getting rid of the session.
> [Ahmed] In our proprietary API the stream and session are one. A session
> holds many properties like the op-type, instead of having this
> information in the op itself. This way we lower the per op setup cost.
> This also allows rapid reuse of stateful infrastructure, once a stream
> is closed on a stateful session, the next op (stream) on this session
> reuses the stateful storage. Obviously if a stream is in "pause mode" on
> a session, all following ops that may be unrelated to this
> stream/session must also wait until this current stream is closed or
> aborted before the infrastructure can be reused.
> >
> > We also struggle with the idea of setting up a stream for stateless ops.
> > - Well, really I just think the name is misleading, i.e. there's no
> > problem with setting
> > up some private PMD data to use with stateless operations, just calling
> > it a
> > stream doesn't seem right.
> [Ahmed] I agree. The op has all the necessary information to process it
> in the current API? Both the stream and the op are one time use. We
> can't attach multiple similar ops to a single stream/session and rely on
> their properties to simplify op setup, so why the hassle.
> >
> > So putting above thoughts together I want to propose:
> > - Removal of the session and all associated APIs.
> > - Passing in one of three data types in the rte_comp_op
> >
> > union {
> > struct rte_comp_xform *xform;
> > /**< Immutable compress/decompress params */
> > void *pmd_stateless_data;
> > /**< Stateless private PMD data derived from an rte_comp_xform
> > * rte_comp_stateless_data_init() must be called on a device
> > * before sending any STATELESS operations. If the PMD returns a
> > non-NULL
> > * value the handle must be attached to subsequent STATELESS
> > operations.
> > * If a PMD returns NULL, then the xform should be passed directly
> > to each op
> > */
> > void *stream;
> > /* Private PMD data derived initially from an rte_comp_xform, which
> > holds state
> > * and history data and evolves as operations are processed.
> > * rte_comp_stream_create() must be called on a device for all
> > STATEFUL
> > * data streams and the resulting stream attached
> > * to the one or more operations associated with the data stream.
> > * All operations in a stream must be sent to the same device.
> > */
> > }
> [Ahmed] I like this setup, but I am not sure in what cases the xform
> immutable would be
Re: [dpdk-dev] [PATCH] compressdev: implement API
Hi Fiona,
Thanks for starting this discussion. In the current API the user must
make 12 API calls just to get information to compress. Maybe there is a
way to simplify. At least for some use cases (stateless). I think a call
sometime next week would be good to help clarify coalesce some of the
complexity.
I added specific comments inline.
Thanks,
Ahmed
On 2/21/2018 2:12 PM, Trahe, Fiona wrote:
> We've been struggling with the idea of session in compressdev.
>
> Is it really a session?
> - It's not in the same sense as cryptodev where it's used to hold a key, and
> maps to a Security Association.
> - It's a set of immutable data that is needed with the op and stream to
> perform the operation.
> - It inherited from cryptodev the ability to be set up for multiple driver
> types and used across any
> devices of those types. For stateful ops this facility can't be used.
> For stateless we don't think it's important, and think it's unlikely to
> be used.
> - Drivers use it to prepare private data, set up resources, do pre-work, so
> there's
> less work to be done on the data path. Initially we didn't have a stream,
> we do now,
> this may be a better alternative place for that work.
> So we've been toying with the idea of getting rid of the session.
[Ahmed] In our proprietary API the stream and session are one. A session
holds many properties like the op-type, instead of having this
information in the op itself. This way we lower the per op setup cost.
This also allows rapid reuse of stateful infrastructure, once a stream
is closed on a stateful session, the next op (stream) on this session
reuses the stateful storage. Obviously if a stream is in "pause mode" on
a session, all following ops that may be unrelated to this
stream/session must also wait until this current stream is closed or
aborted before the infrastructure can be reused.
>
> We also struggle with the idea of setting up a stream for stateless ops.
> - Well, really I just think the name is misleading, i.e. there's no problem
> with setting
> up some private PMD data to use with stateless operations, just calling
> it a
> stream doesn't seem right.
[Ahmed] I agree. The op has all the necessary information to process it
in the current API? Both the stream and the op are one time use. We
can't attach multiple similar ops to a single stream/session and rely on
their properties to simplify op setup, so why the hassle.
>
> So putting above thoughts together I want to propose:
> - Removal of the session and all associated APIs.
> - Passing in one of three data types in the rte_comp_op
>
> union {
> struct rte_comp_xform *xform;
> /**< Immutable compress/decompress params */
> void *pmd_stateless_data;
> /**< Stateless private PMD data derived from an rte_comp_xform
> * rte_comp_stateless_data_init() must be called on a device
> * before sending any STATELESS operations. If the PMD returns a
> non-NULL
> * value the handle must be attached to subsequent STATELESS
> operations.
> * If a PMD returns NULL, then the xform should be passed directly to
> each op
> */
> void *stream;
> /* Private PMD data derived initially from an rte_comp_xform, which
> holds state
> * and history data and evolves as operations are processed.
> * rte_comp_stream_create() must be called on a device for all
> STATEFUL
> * data streams and the resulting stream attached
> * to the one or more operations associated with the data stream.
> * All operations in a stream must be sent to the same device.
> */
> }
[Ahmed] I like this setup, but I am not sure in what cases the xform
immutable would be used. I understand the other two.
> Notes:
> 1. Internally if a PMD wants to use the exact same data structure for both it
> can do,
> just on the API I think it's better if they're named differently with
> different comments.
> 2. I'm not clear of the constraints if any, which attach to the
> pmd_stateless_data
> For our PMD it would only hold immutable data as the session did, and so
> could be attached to many ops in parallel.
> Is this true for all PMDs or are there constraints which should be
> called out?
> Is it limited to a specific device, qp, or to be used on one op at a
> time?
> 3. Am open to other naming suggestions, just trying to capture the essence
> of these data structs better than our current API does.
>
> We would put some more helper fns and structure around the above code if
> people
> are in agreement, just want to see if the concept flies before going further?
>
> Fiona
>
>
>
Re: [dpdk-dev] [PATCH] compressdev: implement API
We've been struggling with the idea of session in compressdev.
Is it really a session?
- It's not in the same sense as cryptodev where it's used to hold a key, and
maps to a Security Association.
- It's a set of immutable data that is needed with the op and stream to
perform the operation.
- It inherited from cryptodev the ability to be set up for multiple driver
types and used across any
devices of those types. For stateful ops this facility can't be used.
For stateless we don't think it's important, and think it's unlikely to be
used.
- Drivers use it to prepare private data, set up resources, do pre-work, so
there's
less work to be done on the data path. Initially we didn't have a stream,
we do now,
this may be a better alternative place for that work.
So we've been toying with the idea of getting rid of the session.
We also struggle with the idea of setting up a stream for stateless ops.
- Well, really I just think the name is misleading, i.e. there's no problem
with setting
up some private PMD data to use with stateless operations, just calling it a
stream doesn't seem right.
So putting above thoughts together I want to propose:
- Removal of the session and all associated APIs.
- Passing in one of three data types in the rte_comp_op
union {
struct rte_comp_xform *xform;
/**< Immutable compress/decompress params */
void *pmd_stateless_data;
/**< Stateless private PMD data derived from an rte_comp_xform
* rte_comp_stateless_data_init() must be called on a device
* before sending any STATELESS operations. If the PMD returns a
non-NULL
* value the handle must be attached to subsequent STATELESS operations.
* If a PMD returns NULL, then the xform should be passed directly to
each op
*/
void *stream;
/* Private PMD data derived initially from an rte_comp_xform, which
holds state
* and history data and evolves as operations are processed.
* rte_comp_stream_create() must be called on a device for all STATEFUL
* data streams and the resulting stream attached
* to the one or more operations associated with the data stream.
* All operations in a stream must be sent to the same device.
*/
}
Notes:
1. Internally if a PMD wants to use the exact same data structure for both it
can do,
just on the API I think it's better if they're named differently with
different comments.
2. I'm not clear of the constraints if any, which attach to the
pmd_stateless_data
For our PMD it would only hold immutable data as the session did, and so
could be attached to many ops in parallel.
Is this true for all PMDs or are there constraints which should be called
out?
Is it limited to a specific device, qp, or to be used on one op at a time?
3. Am open to other naming suggestions, just trying to capture the essence
of these data structs better than our current API does.
We would put some more helper fns and structure around the above code if people
are in agreement, just want to see if the concept flies before going further?
Fiona
Re: [dpdk-dev] [PATCH] compressdev: implement API
02/02/2018 19:25, Fiona Trahe: > config/common_base | 6 + > doc/api/doxy-api-index.md | 1 + > doc/api/doxy-api.conf | 1 + > lib/Makefile | 3 + > lib/librte_compressdev/Makefile| 29 + > lib/librte_compressdev/rte_comp.h | 503 Why rte_comp.h instead of the more consistent rte_compress.h? > lib/librte_compressdev/rte_compressdev.c | 902 > + > lib/librte_compressdev/rte_compressdev.h | 757 + > lib/librte_compressdev/rte_compressdev_pmd.c | 163 > lib/librte_compressdev/rte_compressdev_pmd.h | 439 ++ > lib/librte_compressdev/rte_compressdev_version.map | 47 ++ > lib/librte_eal/common/include/rte_log.h| 1 + > mk/rte.app.mk | 1 + > 13 files changed, 2853 insertions(+) Please update MAINTAINERS file and release notes. Maybe it is worth splitting this patch in few shorter parts? > --- a/config/common_base > +++ b/config/common_base > @@ -535,6 +535,12 @@ CONFIG_RTE_LIBRTE_PMD_MRVL_CRYPTO=n > CONFIG_RTE_LIBRTE_PMD_MRVL_CRYPTO_DEBUG=n > > # > +# Compile generic compression device library > +# > +CONFIG_RTE_LIBRTE_COMPRESSDEV=y > +CONFIG_RTE_COMPRESS_MAX_DEVS=64 > + > +# > # Compile generic security library > # > CONFIG_RTE_LIBRTE_SECURITY=y > diff --git a/doc/api/doxy-api-index.md b/doc/api/doxy-api-index.md > index d77f205..07b8e75 100644 > --- a/doc/api/doxy-api-index.md > +++ b/doc/api/doxy-api-index.md > @@ -43,6 +43,7 @@ The public API headers are grouped by topics: >[rte_tm] (@ref rte_tm.h), >[rte_mtr](@ref rte_mtr.h), >[bbdev] (@ref rte_bbdev.h), > + [compressdev](@ref rte_compressdev.h), >[cryptodev] (@ref rte_cryptodev.h), >[security] (@ref rte_security.h), >[eventdev] (@ref rte_eventdev.h), Please move it between security and eventdev in these lists.
Re: [dpdk-dev] [PATCH] compressdev: implement API
02/02/2018 19:25, Fiona Trahe: > - Used dynamic logging [...] > --- a/lib/librte_eal/common/include/rte_log.h > +++ b/lib/librte_eal/common/include/rte_log.h > @@ -60,6 +60,7 @@ extern struct rte_logs rte_logs; > #define RTE_LOGTYPE_EFD 18 /**< Log related to EFD. */ > #define RTE_LOGTYPE_EVENTDEV 19 /**< Log related to eventdev. */ > #define RTE_LOGTYPE_GSO 20 /**< Log related to GSO. */ > +#define RTE_LOGTYPE_COMPRESSDEV 21 /**< Log related to compressdev. */ You don't need this define with dynamic logging.
[dpdk-dev] [PATCH] compressdev: implement API
With the vast amounts of data being transported around networks and stored in storage systems, reducing data size is becoming ever more important. There are both software libraries and hardware devices available that provide compression, but no common API. This API is proposed in this commit, which supports the following features: - Deflate Algorithm (https://tools.ietf.org/html/rfc1951) - LZS algorithm (https://tools.ietf.org/html/rfc2395) - Static and Dynamic Huffman encoding. - Compression levels - Checksum generation - Asynchronous burst API - Session-based (a session contains immutable data only and is useable across devices) - stream-based to maintain state and history data for stateful flows. Signed-off-by: Fiona Trahe Signed-off-by: Pablo de Lara --- Changes since RFCv3: - Added missing dependencies for shared lib - Used SPDX license header - Used dynamic logging - Changed window size type to uint32_t - Removed some unnecessary API - Replaced phys_addr_t with rte_iova_t - Resolved checkpatch issues - Resolved Doxygen issues - Added default mx nb qps for virtual pmds - Removed unnecessary extern keywords - Resolved enqueue/dequeued prototype issue - Resolved capabilities issues - Completed API documentation - Added missing API in version.map file - Added experimental tag - Added compressdev Doxygen doc in index - Deleted TODOs - Deleted unused event callback mechanism - Clarified flush flag and window size behaviour - Added capability RTE_COMP_FF_NONCOMPRESSED_BLOCKS - Simplified xform struct - no need for separate common and stateful structs - Renamed device features to COMPDEV to distinguish from service features Items to be addressed in v2  - Add hash feature  - Implement stream functions  - Add int rte_comp_stream_create_in_op_priv()  - Add meson build  - Set/clear cache-aligned keyword  - Description of stateless/stateful behaviour in rte_comp_enqueue_burst function header - Add capability helper functions config/common_base | 6 + doc/api/doxy-api-index.md | 1 + doc/api/doxy-api.conf | 1 + lib/Makefile | 3 + lib/librte_compressdev/Makefile| 29 + lib/librte_compressdev/rte_comp.h | 503 lib/librte_compressdev/rte_compressdev.c | 902 + lib/librte_compressdev/rte_compressdev.h | 757 + lib/librte_compressdev/rte_compressdev_pmd.c | 163 lib/librte_compressdev/rte_compressdev_pmd.h | 439 ++ lib/librte_compressdev/rte_compressdev_version.map | 47 ++ lib/librte_eal/common/include/rte_log.h| 1 + mk/rte.app.mk | 1 + 13 files changed, 2853 insertions(+) create mode 100644 lib/librte_compressdev/Makefile create mode 100644 lib/librte_compressdev/rte_comp.h create mode 100644 lib/librte_compressdev/rte_compressdev.c create mode 100644 lib/librte_compressdev/rte_compressdev.h create mode 100644 lib/librte_compressdev/rte_compressdev_pmd.c create mode 100644 lib/librte_compressdev/rte_compressdev_pmd.h create mode 100644 lib/librte_compressdev/rte_compressdev_version.map diff --git a/config/common_base b/config/common_base index ad03cf4..e0e5768 100644 --- a/config/common_base +++ b/config/common_base @@ -535,6 +535,12 @@ CONFIG_RTE_LIBRTE_PMD_MRVL_CRYPTO=n CONFIG_RTE_LIBRTE_PMD_MRVL_CRYPTO_DEBUG=n # +# Compile generic compression device library +# +CONFIG_RTE_LIBRTE_COMPRESSDEV=y +CONFIG_RTE_COMPRESS_MAX_DEVS=64 + +# # Compile generic security library # CONFIG_RTE_LIBRTE_SECURITY=y diff --git a/doc/api/doxy-api-index.md b/doc/api/doxy-api-index.md index d77f205..07b8e75 100644 --- a/doc/api/doxy-api-index.md +++ b/doc/api/doxy-api-index.md @@ -43,6 +43,7 @@ The public API headers are grouped by topics: [rte_tm] (@ref rte_tm.h), [rte_mtr](@ref rte_mtr.h), [bbdev] (@ref rte_bbdev.h), + [compressdev](@ref rte_compressdev.h), [cryptodev] (@ref rte_cryptodev.h), [security] (@ref rte_security.h), [eventdev] (@ref rte_eventdev.h), diff --git a/doc/api/doxy-api.conf b/doc/api/doxy-api.conf index cda52fd..06432c3 100644 --- a/doc/api/doxy-api.conf +++ b/doc/api/doxy-api.conf @@ -45,6 +45,7 @@ INPUT = doc/api/doxy-api-index.md \ lib/librte_cfgfile \ lib/librte_cmdline \ lib/librte_compat \ + lib/librte_compressdev \ lib/librte_cryptodev \ lib/librte_distributor \ lib/librte_efd \ diff --git a/lib/Makefile b/lib/Makefile index ec965a6..19396da 100644 --- a/lib/Makefile +++ b/lib/Makefile @@ -26,6 +26,9 @@ DEPDIRS-librte_bbdev := librte_eal librte_mempool librte_m
