diff mbox

[dpdk-dev,2/3] net/ixgbe: fix build issue

Message ID 1509013365-13819-3-git-send-email-radu.nicolau@intel.com (mailing list archive)
State Superseded, archived
Headers

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation success Compilation OK

Commit Message

Radu Nicolau Oct. 26, 2017, 10:22 a.m. UTC 
  Build fails when rte_security is disabled; make rte_security mandatory
Fixes: 9a0752f498d2 ("net/ixgbe: enable inline IPsec")

Signed-off-by: Radu Nicolau <radu.nicolau@intel.com>
---
 drivers/net/ixgbe/Makefile | 6 ++++++
 1 file changed, 6 insertions(+)

Comments

David Marchand Oct. 26, 2017, 10:36 a.m. UTC | #1 
On Thu, Oct 26, 2017 at 12:22 PM, Radu Nicolau <radu.nicolau@intel.com> wrote:
> Build fails when rte_security is disabled; make rte_security mandatory
> Fixes: 9a0752f498d2 ("net/ixgbe: enable inline IPsec")
>
> Signed-off-by: Radu Nicolau <radu.nicolau@intel.com>
> ---
>  drivers/net/ixgbe/Makefile | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/drivers/net/ixgbe/Makefile b/drivers/net/ixgbe/Makefile
> index f03c426..c879828 100644
> --- a/drivers/net/ixgbe/Makefile
> +++ b/drivers/net/ixgbe/Makefile
> @@ -31,6 +31,12 @@
>
>  include $(RTE_SDK)/mk/rte.vars.mk
>
> +ifneq ($(MAKECMDGOALS),clean)
> +ifneq ($(CONFIG_RTE_LIBRTE_SECURITY),y)
> +$(error "RTE_LIBRTE_SECURITY is required to build RTE_LIBRTE_IXGBE_PMD")
> +endif
> +endif
> +
>  #
>  # library name
>  #

This is a no go for me unless you explain how it is impossible to
disable it in the code.
Radu Nicolau Oct. 26, 2017, 11:01 a.m. UTC | #2 
On 10/26/2017 11:36 AM, David Marchand wrote:
> On Thu, Oct 26, 2017 at 12:22 PM, Radu Nicolau <radu.nicolau@intel.com> wrote:
>> Build fails when rte_security is disabled; make rte_security mandatory
>> Fixes: 9a0752f498d2 ("net/ixgbe: enable inline IPsec")
>>
>> Signed-off-by: Radu Nicolau <radu.nicolau@intel.com>
>> ---
>>   drivers/net/ixgbe/Makefile | 6 ++++++
>>   1 file changed, 6 insertions(+)
>>
>> diff --git a/drivers/net/ixgbe/Makefile b/drivers/net/ixgbe/Makefile
>> index f03c426..c879828 100644
>> --- a/drivers/net/ixgbe/Makefile
>> +++ b/drivers/net/ixgbe/Makefile
>> @@ -31,6 +31,12 @@
>>
>>   include $(RTE_SDK)/mk/rte.vars.mk
>>
>> +ifneq ($(MAKECMDGOALS),clean)
>> +ifneq ($(CONFIG_RTE_LIBRTE_SECURITY),y)
>> +$(error "RTE_LIBRTE_SECURITY is required to build RTE_LIBRTE_IXGBE_PMD")
>> +endif
>> +endif
>> +
>>   #
>>   # library name
>>   #
> This is a no go for me unless you explain how it is impossible to
> disable it in the code.
>
>
It can be disabled in the code, but as far as I know there is a general 
push back against having conditionally compiled code. I originally had 
the security sections in ixgbe PMD isolated, but the feedback was to 
have them always on.
An alternative solution will be to remove the option altogether and 
always build rte_security library.
David Marchand Oct. 26, 2017, 11:27 a.m. UTC | #3 
On Thu, Oct 26, 2017 at 1:01 PM, Radu Nicolau <radu.nicolau@intel.com> wrote:
> On 10/26/2017 11:36 AM, David Marchand wrote:
>> On Thu, Oct 26, 2017 at 12:22 PM, Radu Nicolau <radu.nicolau@intel.com>
>> wrote:
>>>
>>> Build fails when rte_security is disabled; make rte_security mandatory
>>> Fixes: 9a0752f498d2 ("net/ixgbe: enable inline IPsec")
>>>
>>> Signed-off-by: Radu Nicolau <radu.nicolau@intel.com>
>>> ---
>>>   drivers/net/ixgbe/Makefile | 6 ++++++
>>>   1 file changed, 6 insertions(+)
>>>
>>> diff --git a/drivers/net/ixgbe/Makefile b/drivers/net/ixgbe/Makefile
>>> index f03c426..c879828 100644
>>> --- a/drivers/net/ixgbe/Makefile
>>> +++ b/drivers/net/ixgbe/Makefile
>>> @@ -31,6 +31,12 @@
>>>
>>>   include $(RTE_SDK)/mk/rte.vars.mk
>>>
>>> +ifneq ($(MAKECMDGOALS),clean)
>>> +ifneq ($(CONFIG_RTE_LIBRTE_SECURITY),y)
>>> +$(error "RTE_LIBRTE_SECURITY is required to build RTE_LIBRTE_IXGBE_PMD")
>>> +endif
>>> +endif
>>> +
>>>   #
>>>   # library name
>>>   #
>>
>> This is a no go for me unless you explain how it is impossible to
>> disable it in the code.
>>
>>
> It can be disabled in the code, but as far as I know there is a general push
> back against having conditionally compiled code. I originally had the
> security sections in ixgbe PMD isolated, but the feedback was to have them
> always on.

In my mind, this was to stop having features enabled per pmd (and stop
the nightmare with 10 options in a pmd).
Having features globally enabled for all or nothing is still
acceptable, is it not ?


> An alternative solution will be to remove the option altogether and always
> build rte_security library.

As a general rule, I prefer enabling only the things I use, but I am
not against this ?
Can you confirm the performance impact is negligible, always having
this in the pmds ?


Thanks.
Ananyev, Konstantin Oct. 26, 2017, 11:30 a.m. UTC | #4 
> -----Original Message-----

> From: Nicolau, Radu

> Sent: Thursday, October 26, 2017 12:01 PM

> To: David Marchand <david.marchand@6wind.com>

> Cc: dev@dpdk.org; Doherty, Declan <declan.doherty@intel.com>; De Lara Guarch, Pablo <pablo.de.lara.guarch@intel.com>; Hemant

> Agrawal <hemant.agrawal@nxp.com>; borisp@mellanox.com; aviadye@mellanox.com; Thomas Monjalon <thomas@monjalon.net>;

> sandeep.malik@nxp.com; Jerin Jacob <jerin.jacob@caviumnetworks.com>; Mcnamara, John <john.mcnamara@intel.com>; Ananyev,

> Konstantin <konstantin.ananyev@intel.com>; shahafs@mellanox.com; Olivier Matz <olivier.matz@6wind.com>; Akhil Goyal

> <akhil.goyal@nxp.com>

> Subject: Re: [PATCH 2/3] net/ixgbe: fix build issue

> 

> 

> 

> On 10/26/2017 11:36 AM, David Marchand wrote:

> > On Thu, Oct 26, 2017 at 12:22 PM, Radu Nicolau <radu.nicolau@intel.com> wrote:

> >> Build fails when rte_security is disabled; make rte_security mandatory

> >> Fixes: 9a0752f498d2 ("net/ixgbe: enable inline IPsec")

> >>

> >> Signed-off-by: Radu Nicolau <radu.nicolau@intel.com>

> >> ---

> >>   drivers/net/ixgbe/Makefile | 6 ++++++

> >>   1 file changed, 6 insertions(+)

> >>

> >> diff --git a/drivers/net/ixgbe/Makefile b/drivers/net/ixgbe/Makefile

> >> index f03c426..c879828 100644

> >> --- a/drivers/net/ixgbe/Makefile

> >> +++ b/drivers/net/ixgbe/Makefile

> >> @@ -31,6 +31,12 @@

> >>

> >>   include $(RTE_SDK)/mk/rte.vars.mk

> >>

> >> +ifneq ($(MAKECMDGOALS),clean)

> >> +ifneq ($(CONFIG_RTE_LIBRTE_SECURITY),y)

> >> +$(error "RTE_LIBRTE_SECURITY is required to build RTE_LIBRTE_IXGBE_PMD")

> >> +endif

> >> +endif

> >> +

> >>   #

> >>   # library name

> >>   #

> > This is a no go for me unless you explain how it is impossible to

> > disable it in the code.

> >

> >

> It can be disabled in the code, but as far as I know there is a general

> push back against having conditionally compiled code. I originally had

> the security sections in ixgbe PMD isolated, but the feedback was to

> have them always on.

> An alternative solution will be to remove the option altogether and

> always build rte_security library.


My vote would be to have it a mandatory library for ixgbe.
Add it into DEPDIRS-ixgbe inside drivers/net/Makefile or so.
Konstantin
David Marchand Oct. 26, 2017, 11:39 a.m. UTC | #5 
On Thu, Oct 26, 2017 at 1:30 PM, Ananyev, Konstantin
<konstantin.ananyev@intel.com> wrote:
>> -----Original Message-----
>> From: Nicolau, Radu
>> It can be disabled in the code, but as far as I know there is a general
>> push back against having conditionally compiled code. I originally had
>> the security sections in ixgbe PMD isolated, but the feedback was to
>> have them always on.
>> An alternative solution will be to remove the option altogether and
>> always build rte_security library.
>
> My vote would be to have it a mandatory library for ixgbe.
> Add it into DEPDIRS-ixgbe inside drivers/net/Makefile or so.

And then librte_security needs librte_crypto (if I am not mistaken).
So if we go this way, we must ensure the same is done in librte_security.
Thomas Monjalon Oct. 26, 2017, 11:39 a.m. UTC | #6 
26/10/2017 13:27, David Marchand:
> On Thu, Oct 26, 2017 at 1:01 PM, Radu Nicolau <radu.nicolau@intel.com> wrote:
> > On 10/26/2017 11:36 AM, David Marchand wrote:
> >> On Thu, Oct 26, 2017 at 12:22 PM, Radu Nicolau <radu.nicolau@intel.com>
> >> wrote:
> >>> --- a/drivers/net/ixgbe/Makefile
> >>> +++ b/drivers/net/ixgbe/Makefile
> >>> +ifneq ($(MAKECMDGOALS),clean)
> >>> +ifneq ($(CONFIG_RTE_LIBRTE_SECURITY),y)
> >>> +$(error "RTE_LIBRTE_SECURITY is required to build RTE_LIBRTE_IXGBE_PMD")
> >>> +endif
> >>> +endif
> >>
> >> This is a no go for me unless you explain how it is impossible to
> >> disable it in the code.
> >>
> >>
> > It can be disabled in the code, but as far as I know there is a general push
> > back against having conditionally compiled code. I originally had the
> > security sections in ixgbe PMD isolated, but the feedback was to have them
> > always on.
> 
> In my mind, this was to stop having features enabled per pmd (and stop
> the nightmare with 10 options in a pmd).
> Having features globally enabled for all or nothing is still
> acceptable, is it not ?

Yes there is a config option for rte_security,
and it is acceptable.
The code depending on it must be ifdef'ed.
Radu Nicolau Oct. 26, 2017, 12:28 p.m. UTC | #7 
On 10/26/2017 12:39 PM, Thomas Monjalon wrote:
> 26/10/2017 13:27, David Marchand:
>> On Thu, Oct 26, 2017 at 1:01 PM, Radu Nicolau <radu.nicolau@intel.com> wrote:
>>> On 10/26/2017 11:36 AM, David Marchand wrote:
>>>> On Thu, Oct 26, 2017 at 12:22 PM, Radu Nicolau <radu.nicolau@intel.com>
>>>> wrote:
>>>>> --- a/drivers/net/ixgbe/Makefile
>>>>> +++ b/drivers/net/ixgbe/Makefile
>>>>> +ifneq ($(MAKECMDGOALS),clean)
>>>>> +ifneq ($(CONFIG_RTE_LIBRTE_SECURITY),y)
>>>>> +$(error "RTE_LIBRTE_SECURITY is required to build RTE_LIBRTE_IXGBE_PMD")
>>>>> +endif
>>>>> +endif
>>>> This is a no go for me unless you explain how it is impossible to
>>>> disable it in the code.
>>>>
>>>>
>>> It can be disabled in the code, but as far as I know there is a general push
>>> back against having conditionally compiled code. I originally had the
>>> security sections in ixgbe PMD isolated, but the feedback was to have them
>>> always on.
>> In my mind, this was to stop having features enabled per pmd (and stop
>> the nightmare with 10 options in a pmd).
>> Having features globally enabled for all or nothing is still
>> acceptable, is it not ?
> Yes there is a config option for rte_security,
> and it is acceptable.
> The code depending on it must be ifdef'ed.

Given that both ixgbe and dpaa2_sec are now security enabled PMDs, I 
would go with Konstantin's proposal, have rte_security listed as a 
dependency (instead of the explicit check).
Any other PMD is not affected.
For ipsec sample I would keep the explicit check.
Radu Nicolau Oct. 26, 2017, 12:30 p.m. UTC | #8 
On 10/26/2017 12:39 PM, David Marchand wrote:
> On Thu, Oct 26, 2017 at 1:30 PM, Ananyev, Konstantin
> <konstantin.ananyev@intel.com> wrote:
>>> -----Original Message-----
>>> From: Nicolau, Radu
>>> It can be disabled in the code, but as far as I know there is a general
>>> push back against having conditionally compiled code. I originally had
>>> the security sections in ixgbe PMD isolated, but the feedback was to
>>> have them always on.
>>> An alternative solution will be to remove the option altogether and
>>> always build rte_security library.
>> My vote would be to have it a mandatory library for ixgbe.
>> Add it into DEPDIRS-ixgbe inside drivers/net/Makefile or so.
> And then librte_security needs librte_crypto (if I am not mistaken).
> So if we go this way, we must ensure the same is done in librte_security.
>
>
It is already there: DEPDIRS-librte_security += librte_cryptodev
Ananyev, Konstantin Oct. 26, 2017, 12:33 p.m. UTC | #9 
> -----Original Message-----

> From: David Marchand [mailto:david.marchand@6wind.com]

> Sent: Thursday, October 26, 2017 12:39 PM

> To: Ananyev, Konstantin <konstantin.ananyev@intel.com>

> Cc: Nicolau, Radu <radu.nicolau@intel.com>; dev@dpdk.org; Doherty, Declan <declan.doherty@intel.com>; De Lara Guarch, Pablo

> <pablo.de.lara.guarch@intel.com>; Hemant Agrawal <hemant.agrawal@nxp.com>; borisp@mellanox.com; aviadye@mellanox.com;

> Thomas Monjalon <thomas@monjalon.net>; sandeep.malik@nxp.com; Jerin Jacob <jerin.jacob@caviumnetworks.com>; Mcnamara,

> John <john.mcnamara@intel.com>; shahafs@mellanox.com; Olivier Matz <olivier.matz@6wind.com>; Akhil Goyal

> <akhil.goyal@nxp.com>

> Subject: Re: [PATCH 2/3] net/ixgbe: fix build issue

> 

> On Thu, Oct 26, 2017 at 1:30 PM, Ananyev, Konstantin

> <konstantin.ananyev@intel.com> wrote:

> >> -----Original Message-----

> >> From: Nicolau, Radu

> >> It can be disabled in the code, but as far as I know there is a general

> >> push back against having conditionally compiled code. I originally had

> >> the security sections in ixgbe PMD isolated, but the feedback was to

> >> have them always on.

> >> An alternative solution will be to remove the option altogether and

> >> always build rte_security library.

> >

> > My vote would be to have it a mandatory library for ixgbe.

> > Add it into DEPDIRS-ixgbe inside drivers/net/Makefile or so.

> 

> And then librte_security needs librte_crypto (if I am not mistaken).


It seems like it does...
Though from what I understand - it uses inly some struct definitions from it.
Wonder could we move these definitions into rte_security instead and make
rte_cryptodev depend on it instead?
Konstantin

> So if we go this way, we must ensure the same is done in librte_security.

> 

> 

> --

> David Marchand
Thomas Monjalon Oct. 26, 2017, 12:33 p.m. UTC | #10 
26/10/2017 14:28, Radu Nicolau:
> 
> On 10/26/2017 12:39 PM, Thomas Monjalon wrote:
> > 26/10/2017 13:27, David Marchand:
> >> On Thu, Oct 26, 2017 at 1:01 PM, Radu Nicolau <radu.nicolau@intel.com> wrote:
> >>> On 10/26/2017 11:36 AM, David Marchand wrote:
> >>>> On Thu, Oct 26, 2017 at 12:22 PM, Radu Nicolau <radu.nicolau@intel.com>
> >>>> wrote:
> >>>>> --- a/drivers/net/ixgbe/Makefile
> >>>>> +++ b/drivers/net/ixgbe/Makefile
> >>>>> +ifneq ($(MAKECMDGOALS),clean)
> >>>>> +ifneq ($(CONFIG_RTE_LIBRTE_SECURITY),y)
> >>>>> +$(error "RTE_LIBRTE_SECURITY is required to build RTE_LIBRTE_IXGBE_PMD")
> >>>>> +endif
> >>>>> +endif
> >>>> This is a no go for me unless you explain how it is impossible to
> >>>> disable it in the code.
> >>>>
> >>>>
> >>> It can be disabled in the code, but as far as I know there is a general push
> >>> back against having conditionally compiled code. I originally had the
> >>> security sections in ixgbe PMD isolated, but the feedback was to have them
> >>> always on.
> >> In my mind, this was to stop having features enabled per pmd (and stop
> >> the nightmare with 10 options in a pmd).
> >> Having features globally enabled for all or nothing is still
> >> acceptable, is it not ?
> > Yes there is a config option for rte_security,
> > and it is acceptable.
> > The code depending on it must be ifdef'ed.
> 
> Given that both ixgbe and dpaa2_sec are now security enabled PMDs, I 
> would go with Konstantin's proposal, have rte_security listed as a 
> dependency (instead of the explicit check).

Please consider my request instead.
Until now we are ifdef'ing code to allow disabling any lib.
We are not going to change our mind during the last days of a release.
Please just fix it for now.
Akhil Goyal Oct. 26, 2017, 12:59 p.m. UTC | #11 
Hi Thomas,

On 10/26/2017 6:03 PM, Thomas Monjalon wrote:
> 26/10/2017 14:28, Radu Nicolau:
>>
>> On 10/26/2017 12:39 PM, Thomas Monjalon wrote:
>>> 26/10/2017 13:27, David Marchand:
>>>> On Thu, Oct 26, 2017 at 1:01 PM, Radu Nicolau <radu.nicolau@intel.com> wrote:
>>>>> On 10/26/2017 11:36 AM, David Marchand wrote:
>>>>>> On Thu, Oct 26, 2017 at 12:22 PM, Radu Nicolau <radu.nicolau@intel.com>
>>>>>> wrote:
>>>>>>> --- a/drivers/net/ixgbe/Makefile
>>>>>>> +++ b/drivers/net/ixgbe/Makefile
>>>>>>> +ifneq ($(MAKECMDGOALS),clean)
>>>>>>> +ifneq ($(CONFIG_RTE_LIBRTE_SECURITY),y)
>>>>>>> +$(error "RTE_LIBRTE_SECURITY is required to build RTE_LIBRTE_IXGBE_PMD")
>>>>>>> +endif
>>>>>>> +endif
>>>>>> This is a no go for me unless you explain how it is impossible to
>>>>>> disable it in the code.
>>>>>>
>>>>>>
>>>>> It can be disabled in the code, but as far as I know there is a general push
>>>>> back against having conditionally compiled code. I originally had the
>>>>> security sections in ixgbe PMD isolated, but the feedback was to have them
>>>>> always on.
>>>> In my mind, this was to stop having features enabled per pmd (and stop
>>>> the nightmare with 10 options in a pmd).
>>>> Having features globally enabled for all or nothing is still
>>>> acceptable, is it not ?
>>> Yes there is a config option for rte_security,
>>> and it is acceptable.
>>> The code depending on it must be ifdef'ed.
>>
>> Given that both ixgbe and dpaa2_sec are now security enabled PMDs, I
>> would go with Konstantin's proposal, have rte_security listed as a
>> dependency (instead of the explicit check).
> 
> Please consider my request instead.
> Until now we are ifdef'ing code to allow disabling any lib.
> We are not going to change our mind during the last days of a release.
> Please just fix it for now.
> 
> 

For dpaa2_sec we do not want to make the driver run without 
rte_security. We do not see people using it without rte_security.
Will take the Makefile changes that Radu has done in 1st patch of this 
series.

-Akhil
Thomas Monjalon Oct. 26, 2017, 1:07 p.m. UTC | #12 
26/10/2017 14:59, Akhil Goyal:
> Hi Thomas,
> 
> On 10/26/2017 6:03 PM, Thomas Monjalon wrote:
> > 26/10/2017 14:28, Radu Nicolau:
> >>
> >> On 10/26/2017 12:39 PM, Thomas Monjalon wrote:
> >>> 26/10/2017 13:27, David Marchand:
> >>>> On Thu, Oct 26, 2017 at 1:01 PM, Radu Nicolau <radu.nicolau@intel.com> wrote:
> >>>>> On 10/26/2017 11:36 AM, David Marchand wrote:
> >>>>>> On Thu, Oct 26, 2017 at 12:22 PM, Radu Nicolau <radu.nicolau@intel.com>
> >>>>>> wrote:
> >>>>>>> --- a/drivers/net/ixgbe/Makefile
> >>>>>>> +++ b/drivers/net/ixgbe/Makefile
> >>>>>>> +ifneq ($(MAKECMDGOALS),clean)
> >>>>>>> +ifneq ($(CONFIG_RTE_LIBRTE_SECURITY),y)
> >>>>>>> +$(error "RTE_LIBRTE_SECURITY is required to build RTE_LIBRTE_IXGBE_PMD")
> >>>>>>> +endif
> >>>>>>> +endif
> >>>>>> This is a no go for me unless you explain how it is impossible to
> >>>>>> disable it in the code.
> >>>>>>
> >>>>>>
> >>>>> It can be disabled in the code, but as far as I know there is a general push
> >>>>> back against having conditionally compiled code. I originally had the
> >>>>> security sections in ixgbe PMD isolated, but the feedback was to have them
> >>>>> always on.
> >>>> In my mind, this was to stop having features enabled per pmd (and stop
> >>>> the nightmare with 10 options in a pmd).
> >>>> Having features globally enabled for all or nothing is still
> >>>> acceptable, is it not ?
> >>> Yes there is a config option for rte_security,
> >>> and it is acceptable.
> >>> The code depending on it must be ifdef'ed.
> >>
> >> Given that both ixgbe and dpaa2_sec are now security enabled PMDs, I
> >> would go with Konstantin's proposal, have rte_security listed as a
> >> dependency (instead of the explicit check).
> > 
> > Please consider my request instead.
> > Until now we are ifdef'ing code to allow disabling any lib.
> > We are not going to change our mind during the last days of a release.
> > Please just fix it for now.
> 
> For dpaa2_sec we do not want to make the driver run without 
> rte_security. We do not see people using it without rte_security.

Why not?

> Will take the Makefile changes that Radu has done in 1st patch of this 
> series.

Is it really a lot to ifdef?

It is going to break compilation of DPDK for those who disable rte_security.
Akhil Goyal Oct. 26, 2017, 1:16 p.m. UTC | #13 
On 10/26/2017 6:37 PM, Thomas Monjalon wrote:
> 26/10/2017 14:59, Akhil Goyal:
>> Hi Thomas,
>>
>> On 10/26/2017 6:03 PM, Thomas Monjalon wrote:
>>> 26/10/2017 14:28, Radu Nicolau:
>>>>
>>>> On 10/26/2017 12:39 PM, Thomas Monjalon wrote:
>>>>> 26/10/2017 13:27, David Marchand:
>>>>>> On Thu, Oct 26, 2017 at 1:01 PM, Radu Nicolau <radu.nicolau@intel.com> wrote:
>>>>>>> On 10/26/2017 11:36 AM, David Marchand wrote:
>>>>>>>> On Thu, Oct 26, 2017 at 12:22 PM, Radu Nicolau <radu.nicolau@intel.com>
>>>>>>>> wrote:
>>>>>>>>> --- a/drivers/net/ixgbe/Makefile
>>>>>>>>> +++ b/drivers/net/ixgbe/Makefile
>>>>>>>>> +ifneq ($(MAKECMDGOALS),clean)
>>>>>>>>> +ifneq ($(CONFIG_RTE_LIBRTE_SECURITY),y)
>>>>>>>>> +$(error "RTE_LIBRTE_SECURITY is required to build RTE_LIBRTE_IXGBE_PMD")
>>>>>>>>> +endif
>>>>>>>>> +endif
>>>>>>>> This is a no go for me unless you explain how it is impossible to
>>>>>>>> disable it in the code.
>>>>>>>>
>>>>>>>>
>>>>>>> It can be disabled in the code, but as far as I know there is a general push
>>>>>>> back against having conditionally compiled code. I originally had the
>>>>>>> security sections in ixgbe PMD isolated, but the feedback was to have them
>>>>>>> always on.
>>>>>> In my mind, this was to stop having features enabled per pmd (and stop
>>>>>> the nightmare with 10 options in a pmd).
>>>>>> Having features globally enabled for all or nothing is still
>>>>>> acceptable, is it not ?
>>>>> Yes there is a config option for rte_security,
>>>>> and it is acceptable.
>>>>> The code depending on it must be ifdef'ed.
>>>>
>>>> Given that both ixgbe and dpaa2_sec are now security enabled PMDs, I
>>>> would go with Konstantin's proposal, have rte_security listed as a
>>>> dependency (instead of the explicit check).
>>>
>>> Please consider my request instead.
>>> Until now we are ifdef'ing code to allow disabling any lib.
>>> We are not going to change our mind during the last days of a release.
>>> Please just fix it for now.
>>
>> For dpaa2_sec we do not want to make the driver run without
>> rte_security. We do not see people using it without rte_security.
> 
> Why not?
We see a lot of performance difference in the two cases. People may not 
like to see a lower performance for the same protocol processing.

> 
>> Will take the Makefile changes that Radu has done in 1st patch of this
>> series.
> 
> Is it really a lot to ifdef?
As I see it would be around 12-13 checks in 2 files.
> 
> It is going to break compilation of DPDK for those who disable rte_security.
> 

Well I would say, if people do not need rte_security then they can 
disable dpaa2_sec_pmd also.

-Akhil
Thomas Monjalon Oct. 26, 2017, 2 p.m. UTC | #14 
26/10/2017 15:16, Akhil Goyal:
> On 10/26/2017 6:37 PM, Thomas Monjalon wrote:
> > 26/10/2017 14:59, Akhil Goyal:
> >> Hi Thomas,
> >>
> >> On 10/26/2017 6:03 PM, Thomas Monjalon wrote:
> >>> 26/10/2017 14:28, Radu Nicolau:
> >>>>
> >>>> On 10/26/2017 12:39 PM, Thomas Monjalon wrote:
> >>>>> 26/10/2017 13:27, David Marchand:
> >>>>>> On Thu, Oct 26, 2017 at 1:01 PM, Radu Nicolau <radu.nicolau@intel.com> wrote:
> >>>>>>> On 10/26/2017 11:36 AM, David Marchand wrote:
> >>>>>>>> On Thu, Oct 26, 2017 at 12:22 PM, Radu Nicolau <radu.nicolau@intel.com>
> >>>>>>>> wrote:
> >>>>>>>>> --- a/drivers/net/ixgbe/Makefile
> >>>>>>>>> +++ b/drivers/net/ixgbe/Makefile
> >>>>>>>>> +ifneq ($(MAKECMDGOALS),clean)
> >>>>>>>>> +ifneq ($(CONFIG_RTE_LIBRTE_SECURITY),y)
> >>>>>>>>> +$(error "RTE_LIBRTE_SECURITY is required to build RTE_LIBRTE_IXGBE_PMD")
> >>>>>>>>> +endif
> >>>>>>>>> +endif
> >>>>>>>> This is a no go for me unless you explain how it is impossible to
> >>>>>>>> disable it in the code.
> >>>>>>>>
> >>>>>>>>
> >>>>>>> It can be disabled in the code, but as far as I know there is a general push
> >>>>>>> back against having conditionally compiled code. I originally had the
> >>>>>>> security sections in ixgbe PMD isolated, but the feedback was to have them
> >>>>>>> always on.
> >>>>>> In my mind, this was to stop having features enabled per pmd (and stop
> >>>>>> the nightmare with 10 options in a pmd).
> >>>>>> Having features globally enabled for all or nothing is still
> >>>>>> acceptable, is it not ?
> >>>>> Yes there is a config option for rte_security,
> >>>>> and it is acceptable.
> >>>>> The code depending on it must be ifdef'ed.
> >>>>
> >>>> Given that both ixgbe and dpaa2_sec are now security enabled PMDs, I
> >>>> would go with Konstantin's proposal, have rte_security listed as a
> >>>> dependency (instead of the explicit check).
> >>>
> >>> Please consider my request instead.
> >>> Until now we are ifdef'ing code to allow disabling any lib.
> >>> We are not going to change our mind during the last days of a release.
> >>> Please just fix it for now.
> >>
> >> For dpaa2_sec we do not want to make the driver run without
> >> rte_security. We do not see people using it without rte_security.
> > 
> > Why not?
> We see a lot of performance difference in the two cases. People may not 
> like to see a lower performance for the same protocol processing.
> 
> > 
> >> Will take the Makefile changes that Radu has done in 1st patch of this
> >> series.
> > 
> > Is it really a lot to ifdef?
> As I see it would be around 12-13 checks in 2 files.
> > 
> > It is going to break compilation of DPDK for those who disable rte_security.
> > 
> 
> Well I would say, if people do not need rte_security then they can 
> disable dpaa2_sec_pmd also.

OK
diff mbox

Patch

diff --git a/drivers/net/ixgbe/Makefile b/drivers/net/ixgbe/Makefile
index f03c426..c879828 100644
--- a/drivers/net/ixgbe/Makefile
+++ b/drivers/net/ixgbe/Makefile
@@ -31,6 +31,12 @@ 
 
 include $(RTE_SDK)/mk/rte.vars.mk
 
+ifneq ($(MAKECMDGOALS),clean)
+ifneq ($(CONFIG_RTE_LIBRTE_SECURITY),y)
+$(error "RTE_LIBRTE_SECURITY is required to build RTE_LIBRTE_IXGBE_PMD")
+endif
+endif
+
 #
 # library name
 #