DPDK patches and discussions
 help / color / mirror / Atom feed
From: Akhil Goyal <gakhil@marvell.com>
To: "Ananyev, Konstantin" <konstantin.ananyev@intel.com>,
	Anoob Joseph <anoobj@marvell.com>,
	"Doherty, Declan" <declan.doherty@intel.com>,
	"Zhang, Roy Fan" <roy.fan.zhang@intel.com>,
	"hemant.agrawal@nxp.com" <hemant.agrawal@nxp.com>
Cc: Jerin Jacob Kollanukkaran <jerinj@marvell.com>,
	Ankur Dwivedi <adwivedi@marvell.com>,
	Tejasree Kondoj <ktejasree@marvell.com>,
	"dev@dpdk.org" <dev@dpdk.org>,
	Archana Muniganti <marchana@marvell.com>
Subject: Re: [dpdk-dev] [PATCH 2/2] lib/security: add SA lifetime configuration
Date: Mon, 26 Jul 2021 15:50:02 +0000	[thread overview]
Message-ID: <CO6PR18MB448449F0CBFF5120967A6970D8E89@CO6PR18MB4484.namprd18.prod.outlook.com> (raw)
In-Reply-To: <DM6PR11MB449186E8C4A7D44873F1CAA79AE89@DM6PR11MB4491.namprd11.prod.outlook.com>

Hi Konstantin,
> > There are two options that we considered,
> > 1. Extend the enum, rte_crypto_op_status,  to cover warnings [1]
> > 2. There are reserved fields in rte_cryto_op structure. So we can use bits in
> them to indicate various cases. [2]
> >
> > Both the submitted patches follow approach 1 (following how it's done
> currently), but we can switch to approach 2 if we think there can be
> > more such "warnings" that can occur simultaneously. Can you share your
> thoughts on how we should extend the library to handle such
> > cases?
> >
> > [1] https://doc.dpdk.org/api/rte__crypto_8h.html#afe16508b77c2a8dc5caf74a4e9850171
> > [2] https://doc.dpdk.org/api/rte__crypto_8h_source.html
> 
> My vote would probably be for option #2 (use one of the reserved fields for
> it).
> That way - existing code wouldn't need to be changed.

Adding a single enum or multiple enums is the same thing. Right wrt code changes?
However, if the check is something like 
If (status != RTE_CRYPTO_OP_STATUS_SUCCESS)
	Report appropriate error number
App code will need to be updated to take care the warnings in both options.
It will be something like
Option #1
If (status != RTE_CRYPTO_OP_STATUS_SUCCESS) {
	If (status < RTE_CRYPTO_OP_STATUS_SUCCESS)
		Report appropriate error number.
	Else
		Report appropriate warning number probably in debug prints.
}
Option #2	
If (op->status != RTE_CRYPTO_OP_STATUS_SUCCESS) {
	If (op->status == RTE_CRYPTO_OP_STATUS_WARNING) {
		Report appropriate warning based on op->reserved[0]
	} else {
		Report appropriate error number
	}
}
Here both the options are same wrt performance.
But in option #2, driver and app need to fill and decode 2 separate variables
As against 1 variable in option #1

In both the options, there will be similar code changes.
Do you suspect any other code change?

> Again these warnings, it probably needs to be a bit-flags, correct?

We can deal with both bit flags as well as new enums in the status.
I believe both are same and in fact using enum in application is more convenient
for user, instead of decoding bit flags.
However, it is personal choice. People may differ on that.

Regards,
Akhil




  reply	other threads:[~2021-07-26 15:50 UTC|newest]

Thread overview: 15+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-07-20  5:46 [dpdk-dev] [PATCH 0/2] Improvements to rte_security Anoob Joseph
2021-07-20  5:46 ` [dpdk-dev] [PATCH 1/2] lib/security: add IV generation Anoob Joseph
2021-07-20  5:46 ` [dpdk-dev] [PATCH 2/2] lib/security: add SA lifetime configuration Anoob Joseph
2021-07-20  6:20   ` Anoob Joseph
2021-07-26 13:50     ` Ananyev, Konstantin
2021-07-26 15:50       ` Akhil Goyal [this message]
2021-07-27 11:40         ` Ananyev, Konstantin
2021-07-27 19:29           ` Akhil Goyal
2021-07-28 10:59             ` Ananyev, Konstantin
2021-07-28 12:58               ` Akhil Goyal
2021-07-28 14:38                 ` Anoob Joseph
2021-07-29 10:23                   ` Ananyev, Konstantin
2021-08-02  7:07                     ` Anoob Joseph
2021-08-03 11:51                       ` Ananyev, Konstantin
2021-08-03 12:03                         ` Anoob Joseph

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=CO6PR18MB448449F0CBFF5120967A6970D8E89@CO6PR18MB4484.namprd18.prod.outlook.com \
    --to=gakhil@marvell.com \
    --cc=adwivedi@marvell.com \
    --cc=anoobj@marvell.com \
    --cc=declan.doherty@intel.com \
    --cc=dev@dpdk.org \
    --cc=hemant.agrawal@nxp.com \
    --cc=jerinj@marvell.com \
    --cc=konstantin.ananyev@intel.com \
    --cc=ktejasree@marvell.com \
    --cc=marchana@marvell.com \
    --cc=roy.fan.zhang@intel.com \
    /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).