DPDK patches and discussions
 help / color / mirror / Atom feed
From: "Gujjar, Abhinandan S" <abhinandan.gujjar@intel.com>
To: Akhil Goyal <gakhil@marvell.com>,
	Anoob Joseph <anoobj@marvell.com>,
	Shijith Thotton <sthotton@marvell.com>
Cc: Jerin Jacob Kollanukkaran <jerinj@marvell.com>,
	"Pavan Nikhilesh Bhagavatula" <pbhagavatula@marvell.com>,
	Ray Kinsella <mdr@ashroe.eu>,
	"Ankur Dwivedi" <adwivedi@marvell.com>,
	"dev@dpdk.org" <dev@dpdk.org>,
	"thomas@monjalon.net" <thomas@monjalon.net>,
	"nipun.gupta@nxp.com" <nipun.gupta@nxp.com>,
	Hemant Agrawal <hemant.agrawal@nxp.com>
Subject: Re: [dpdk-dev] [PATCH v3] eventdev: update crypto adapter metadata structures
Date: Mon, 20 Sep 2021 05:54:51 +0000	[thread overview]
Message-ID: <PH0PR11MB482444DEF8612D7AACCB7A40E8A09@PH0PR11MB4824.namprd11.prod.outlook.com> (raw)
In-Reply-To: <CO6PR18MB4484ACB4016883C9D73CF848D8DF9@CO6PR18MB4484.namprd18.prod.outlook.com>

Hi Akhil,

> -----Original Message-----
> From: Akhil Goyal <gakhil@marvell.com>
> Sent: Monday, September 20, 2021 12:19 AM
> To: Gujjar, Abhinandan S <abhinandan.gujjar@intel.com>; Anoob Joseph
> <anoobj@marvell.com>; Shijith Thotton <sthotton@marvell.com>
> Cc: Jerin Jacob Kollanukkaran <jerinj@marvell.com>; Pavan Nikhilesh
> Bhagavatula <pbhagavatula@marvell.com>; Ray Kinsella <mdr@ashroe.eu>;
> Ankur Dwivedi <adwivedi@marvell.com>; dev@dpdk.org;
> thomas@monjalon.net; nipun.gupta@nxp.com; Hemant Agrawal
> <hemant.agrawal@nxp.com>
> Subject: RE: [PATCH v3] eventdev: update crypto adapter metadata
> structures
> 
> Hi Abhinandan,
> > > > >> >>
> > > > >> >> >> In crypto adapter metadata, reserved bytes in request
> > > > >> >> >> info structure is a space holder for response info. It
> > > > >> >> >> enforces an order of operation if the structures are
> > > > >> >> >> updated using memcpy to avoid overwriting response info.
> > > > >> >> >> It is logical to move the reserved space out of request
> > > > >> >> >> info. It also solves the ordering issue
> > > > >> mentioned before.
> > > > >> >> >I would like to understand what kind of ordering issue you
> > > > >> >> >have faced with the current approach. Could you please give
> > > > >> >> >an example/sequence
> > > > >> >> and explain?
> > > > >> >> >
> > > > >> >>
> > > > >> >> I have seen this issue with crypto adapter autotest (#n215).
> > > > >> >>
> > > > >> >> Example:
> > > > >> >> rte_memcpy(&m_data.response_info, &response_info,
> > > > >> >> sizeof(response_info)); rte_memcpy(&m_data.request_info,
> > > > >> >> &request_info, sizeof(request_info));
> > > > >> >>
> > > > >> >> Here response info is getting overwritten by request info.
> > > > >> >> Above lines can reordered to fix the issue, but can be
> > > > >> >> ignored with this
> > > > >> patch.
> > > > >> >There is a reason for designing the metadata in this way.
> > > > >> >Right now, sizeof (union rte_event_crypto_metadata) is 16 bytes.
> > > > >> >So, the session based case needs just 16 bytes to store the data.
> > > > >> >Whereas, for sessionless case each crypto_ops requires another
> > > > >> >16
> > > > bytes.
> > > > >> >
> > > > >> >By changing the struct in the following way you are doubling
> > > > >> >the memory requirement.
> > > > >> >With the changes, for sessionless case, each crypto op
> > > > >> >requires 32 bytes of space instead of 16 bytes and the mempool
> will be bigger.
> > > > >> >This will have the perf impact too!
> > > > >> >
> > > > >> >You can just copy the individual members(cdev_id &
> > > > >> >queue_pair_id) after the response_info.
> > > > >> >OR You have a better way?
> > > > >> >
> > > > >>
> > > > >> I missed the second word of event structure. It adds an extra 8
> > > > >> bytes, which is not required.
> > > > >I guess you meant not adding, it is overlapping with request
> > information.
> > > > >> Let me know, what you think of below change to metadata
> structure.
> > > > >>
> > > > >> struct rte_event_crypto_metadata {
> > > > >> 	uint64_t response_info; // 8 bytes
> > > > >With this, it lags clarity saying it is an event information.
> > > > >> 	struct rte_event_crypto_request request_info; // 8 bytes };
> > > > >>
> > > > >> Total structure size is 16 bytes.
> > > > >>
> > > > >> Response info can be set like below in test app:
> > > > >> 	m_data.response_info = response_info.event;
> > > > >>
> > > > >> The main aim of this patch is to decouple response info from
> > > > >> request
> > > info.
> > > > >The decoupling was required as it was doing memcpy.
> > > > >If you copy the individual members of request info(after
> > > > >response), you don't require it.
> > > >
> > > > With this change, the application will be free of such constraints.
> > >
> > > [Anoob] Why don't we make it like,
> > >
> > > struct rte_event_crypto_metadata {
> > >  	uint64_t ev_w0_template;
> > >               /**< Template of the first word of ``struct
> > > rte_event``
> > > (rte_event.event) to be set in the events generated by crypto
> > > adapter in both RTE_EVENT_CRYPTO_ADAPTER_OP_NEW and
> > > 	 * RTE_EVENT_CRYPTO_ADAPTER_OP_FORWARD modes.
> > > 	*/
> > >  	struct rte_event_crypto_request request_info; };
> > >
> > > This way, the usage would become more obvious and we will not have
> > > additional space reserved as well.
> >
> > Thanks for the suggestion. At this point, it is like an application
> > without understanding the data structure/ internals has used memcpy
> > and we are trying to fix it by changing the actual data structure
> > instead of fixing the application!
> > With this patch, the other applications(outside of dpdk) which are
> > using the data structures in a right are forced to change!
> > I don't think, this is the right way to handle this. I think, we
> > should be updating the test case and documentation for this rather
> > than changing the data structure.  I expect the next version of this
> > patch should have those changes. Thanks!
> 
> The point here is about making the specification better and clearer to the
> user.
> If the structure is not clear and is error prone if somebody does not follow
> Proper documentation, we can modify it to reduce possible issues.
I think, the structure is clear. Apart from adding documentation,
I feel forming a structure with appropriate data type and naming it to make it self-explanatory is important.
That is what we have done, and this is already discussed in the original patch as well.
It is important for any application writer to go through the documentation and structure to understand before using it.
The current documentation clearing says it has overlap. If you still feel it lags clarity in some places please go ahead and add documentation.
It is not about the ABI breakage; it is about changing a proper structure for an ignorant application!
I am not convinced with the proposed changes. If you have a better one, please let me know.

> 
> This is a cleanup which was added in deprecation notice as well in the last
> release.
> This is a ABI break release and we should do this cleanup if it is legit.
> Applications outside DPDK are notified as per the deprecation notice in the
> last release.
> We have followed standard procedure to modify this structure, hence, no
> need to worry about that.
> We have provided, 2-3 suggestions to clean this up. Please suggest which
> one is best for Intel use case.
> 
> Having first element as reserved in the structure doesn't look quite obvious.
> This was also highlighted in the original patch, but was not taken up seriously
> as we were not supporting it at that point.
> See this
> http://patches.dpdk.org/project/dpdk/patch/1524573807-168522-2-git-
> send-email-abhinandan.gujjar@intel.com/

> 
> -Akhil
> 
> Please note:
> Avoid top posting for comments and remove unnecessary lines while
> commenting back.
> 
Please note that, I am using outlook with appropriate settings to respond back.
I am not sure, why this is coming as top posting.

  reply	other threads:[~2021-09-20  5:54 UTC|newest]

Thread overview: 30+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-08-04 18:14 [dpdk-dev] [PATCH] doc: announce change in crypto adapter metadata Shijith Thotton
2021-08-04 18:17 ` Akhil Goyal
2021-08-05 10:12   ` Kinsella, Ray
2021-08-05 10:21     ` Jerin Jacob
2021-08-07 15:10       ` Thomas Monjalon
2021-08-05  2:53 ` [dpdk-dev] [RFC] eventdev: update crypto adapter metadata structures Shijith Thotton
2021-08-30 19:44   ` [dpdk-dev] [PATCH] " Shijith Thotton
2021-08-30 19:59     ` [dpdk-dev] [PATCH v2] " Shijith Thotton
2021-08-31  4:00       ` Anoob Joseph
2021-08-31  6:08       ` Akhil Goyal
2021-08-31  6:51         ` Shijith Thotton
2021-08-31  7:56       ` [dpdk-dev] [PATCH v3] " Shijith Thotton
2021-09-07  8:34         ` Jerin Jacob
2021-09-07  8:37           ` [dpdk-dev] [EXT] " Akhil Goyal
2021-09-07  8:53         ` [dpdk-dev] " Gujjar, Abhinandan S
2021-09-07 10:37           ` Shijith Thotton
2021-09-07 17:30             ` Gujjar, Abhinandan S
2021-09-08  7:42               ` Shijith Thotton
2021-09-08  7:53                 ` Gujjar, Abhinandan S
2021-09-14  4:36                   ` Shijith Thotton
2021-09-14  4:46                     ` Anoob Joseph
2021-09-19 14:25                       ` Gujjar, Abhinandan S
2021-09-19 18:49                         ` Akhil Goyal
2021-09-20  5:54                           ` Gujjar, Abhinandan S [this message]
2021-09-27 15:22         ` [dpdk-dev] [PATCH v4] doc: remove event crypto metadata deprecation note Shijith Thotton
2021-09-27 15:29           ` [dpdk-dev] [PATCH] test/event_crypto: fix event crypto metadata write Shijith Thotton
2021-10-03  9:46             ` Gujjar, Abhinandan S
2021-10-05 15:15               ` Akhil Goyal
2021-10-03  9:48           ` [dpdk-dev] [PATCH v4] doc: remove event crypto metadata deprecation note Gujjar, Abhinandan S
2021-10-05 14:49             ` Akhil Goyal

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=PH0PR11MB482444DEF8612D7AACCB7A40E8A09@PH0PR11MB4824.namprd11.prod.outlook.com \
    --to=abhinandan.gujjar@intel.com \
    --cc=adwivedi@marvell.com \
    --cc=anoobj@marvell.com \
    --cc=dev@dpdk.org \
    --cc=gakhil@marvell.com \
    --cc=hemant.agrawal@nxp.com \
    --cc=jerinj@marvell.com \
    --cc=mdr@ashroe.eu \
    --cc=nipun.gupta@nxp.com \
    --cc=pbhagavatula@marvell.com \
    --cc=sthotton@marvell.com \
    --cc=thomas@monjalon.net \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).