* Re: [RFC 17.05 v1 0/3] Merge l3fwd-acl and l3fwd
@ 2017-03-05 13:15 Ananyev, Konstantin
2017-03-05 19:55 ` Ravi Kerur
0 siblings, 1 reply; 7+ messages in thread
From: Ananyev, Konstantin @ 2017-03-05 13:15 UTC (permalink / raw)
To: Ravi Kerur (rkerur@gmail.com), dev; +Cc: Richardson, Bruce
Hi Ravi,
>
> Hi Konstantin,
>
> Sorry for this one, I had to resend patch series as 'v3' as additional checkpatch warnings were seen after the submission which didn't show
> up in my run.
>
> 'v3' patch should have all fixed except the ones I mentioned in my earlier email on which I need inputs from you.
>
> Thanks.
>
> On Sat, Mar 4, 2017 at 11:49 AM, Ravi Kerur <rkerur@gmail.com> wrote:
> Hi Konstantin,
>
> I have sent 'v2' patchset. I need clarifications on following things, if they should be fixed I will send out 'v3' so please let me know.
>
> Following code changes were done by me manually, not merged.
> +++ b/examples/l3fwd/main.c
> @@ -161,7 +163,9 @@ static struct rte_eth_conf port_conf = {
> .rx_adv_conf = {
> .rss_conf = {
> .rss_key = NULL,
> - .rss_hf = ETH_RSS_IP,
> + .rss_hf = ETH_RSS_IP | ETH_RSS_UDP |
> + ETH_RSS_TCP | ETH_RSS_SCTP,
> +
> },
>
> The reason I did it is because
>
> LPM/EM has .rss_hf = ETH_RSS_IP
> ACL has .rss_hf = ETH_RSS_IP | ETH_RSS_UDP | ETH_RSS_TCP | ETH_RSS_SCTP,
>
> ACL looks like a superset of LPM/EM and functional testing didn't reveal any issues hence I kept ACL version.
But at least for LPM, we probably don't want L4 ports affect packet distribution?
Probably the safest way would be to have a separate port_conf for each case (LPM/EM/ACL).
That way will preserve the original behavior.
>
> 2. Checkpatch errors are all fixed. Some warnings are not fixed and they are
>
> 2.a, string length greater than 80 characters
> 2.b GET_CB_FIELD macro. I could have changed GET_CB_FIELD to inline function, however, function names cannot be in capital letters. I
> could have changed it to 'get_cb_field' inline function, but didn't do it as I thought it may not be worth the change.
It is ok by me to leave as it is by now.
Thanks
Konstantin
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [RFC 17.05 v1 0/3] Merge l3fwd-acl and l3fwd
2017-03-05 13:15 [RFC 17.05 v1 0/3] Merge l3fwd-acl and l3fwd Ananyev, Konstantin
@ 2017-03-05 19:55 ` Ravi Kerur
0 siblings, 0 replies; 7+ messages in thread
From: Ravi Kerur @ 2017-03-05 19:55 UTC (permalink / raw)
To: Ananyev, Konstantin; +Cc: dev, Richardson, Bruce
Hi Konstantin,
Thanks for the inputs.
Initialization starts with ETH_RSS_IP and gets updated with L4 if ACL is
selected in parse_args function. This change is to fix rss_hf.
I have submitted 'v4' patch, please let me know if additional changes are
needed.
Thanks.
On Sun, Mar 5, 2017 at 5:15 AM, Ananyev, Konstantin <
konstantin.ananyev@intel.com> wrote:
>
> Hi Ravi,
>
> >
> > Hi Konstantin,
> >
> > Sorry for this one, I had to resend patch series as 'v3' as additional
> checkpatch warnings were seen after the submission which didn't show
> > up in my run.
> >
> > 'v3' patch should have all fixed except the ones I mentioned in my
> earlier email on which I need inputs from you.
> >
> > Thanks.
> >
> > On Sat, Mar 4, 2017 at 11:49 AM, Ravi Kerur <rkerur@gmail.com> wrote:
> > Hi Konstantin,
> >
> > I have sent 'v2' patchset. I need clarifications on following things, if
> they should be fixed I will send out 'v3' so please let me know.
> >
> > Following code changes were done by me manually, not merged.
> > +++ b/examples/l3fwd/main.c
> > @@ -161,7 +163,9 @@ static struct rte_eth_conf port_conf = {
> > .rx_adv_conf = {
> > .rss_conf = {
> > .rss_key = NULL,
> > - .rss_hf = ETH_RSS_IP,
> > + .rss_hf = ETH_RSS_IP | ETH_RSS_UDP |
> > + ETH_RSS_TCP | ETH_RSS_SCTP,
> > +
> > },
> >
> > The reason I did it is because
> >
> > LPM/EM has .rss_hf = ETH_RSS_IP
> > ACL has .rss_hf = ETH_RSS_IP | ETH_RSS_UDP | ETH_RSS_TCP | ETH_RSS_SCTP,
> >
> > ACL looks like a superset of LPM/EM and functional testing didn't reveal
> any issues hence I kept ACL version.
>
> But at least for LPM, we probably don't want L4 ports affect packet
> distribution?
> Probably the safest way would be to have a separate port_conf for each
> case (LPM/EM/ACL).
> That way will preserve the original behavior.
>
> >
> > 2. Checkpatch errors are all fixed. Some warnings are not fixed and they
> are
> >
> > 2.a, string length greater than 80 characters
> > 2.b GET_CB_FIELD macro. I could have changed GET_CB_FIELD to inline
> function, however, function names cannot be in capital letters. I
> > could have changed it to 'get_cb_field' inline function, but didn't do
> it as I thought it may not be worth the change.
>
> It is ok by me to leave as it is by now.
> Thanks
> Konstantin
>
>
>
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [RFC 17.05 v1 0/3] Merge l3fwd-acl and l3fwd
2017-03-04 19:49 ` Ravi Kerur
@ 2017-03-04 20:47 ` Ravi Kerur
0 siblings, 0 replies; 7+ messages in thread
From: Ravi Kerur @ 2017-03-04 20:47 UTC (permalink / raw)
To: Ananyev, Konstantin; +Cc: dev, Richardson, Bruce
Hi Konstantin,
Sorry for this one, I had to resend patch series as 'v3' as additional
checkpatch warnings were seen after the submission which didn't show up in
my run.
'v3' patch should have all fixed except the ones I mentioned in my earlier
email on which I need inputs from you.
Thanks.
On Sat, Mar 4, 2017 at 11:49 AM, Ravi Kerur <rkerur@gmail.com> wrote:
> Hi Konstantin,
>
> I have sent 'v2' patchset. I need clarifications on following things, if
> they should be fixed I will send out 'v3' so please let me know.
>
> Following code changes were done by me manually, not merged.
> +++ b/examples/l3fwd/main.c
> @@ -161,7 +163,9 @@ static struct rte_eth_conf port_conf = {
> .rx_adv_conf = {
> .rss_conf = {
> .rss_key = NULL,
> - .rss_hf = ETH_RSS_IP,
> + .rss_hf = ETH_RSS_IP | ETH_RSS_UDP |
> + ETH_RSS_TCP | ETH_RSS_SCTP,
> +
> },
>
> The reason I did it is because
>
> LPM/EM has .rss_hf = ETH_RSS_IP
> ACL has .rss_hf = ETH_RSS_IP | ETH_RSS_UDP | ETH_RSS_TCP | ETH_RSS_SCTP,
>
> ACL looks like a superset of LPM/EM and functional testing didn't reveal
> any issues hence I kept ACL version.
>
> 2. Checkpatch errors are all fixed. Some warnings are not fixed and they
> are
>
> 2.a, string length greater than 80 characters
> 2.b GET_CB_FIELD macro. I could have changed GET_CB_FIELD to inline
> function, however, function names cannot be in capital letters. I could
> have changed it to 'get_cb_field' inline function, but didn't do it as I
> thought it may not be worth the change.
>
> Let me know your inputs.
>
> Thanks.
>
> On Wed, Mar 1, 2017 at 7:29 AM, Ravi Kerur <rkerur@gmail.com> wrote:
>
>> Hi Konstantin,
>>
>> Thank you for the review.
>>
>> RSS hash value changes could be due to merge, I didn't make that change.
>> I will go through the changes and fix it in 'v2' patch along with RFC
>> removed and checkpatch fix.
>>
>> Thanks.
>>
>> On Tue, Feb 28, 2017 at 2:36 AM, Ananyev, Konstantin <
>> konstantin.ananyev@intel.com> wrote:
>>
>>> Hi Ravi,
>>>
>>> >
>>> > Thanks to Konstantin and Bruce on first internal review comments. This
>>> > patch is RFC for 17.05 to merge l3fwd-acl and l3fwd code and add file
>>> > read options to build LPM and EM tables.
>>>
>>>
>>> Thanks for the patch, I think it is really useful one.
>>> Can I suggest you re-submit it as non-RFC now, as we are in 17.05 window
>>> already?
>>> About the patch itself, one question I forgot to ask you before:
>>>
>>> +++ b/examples/l3fwd/main.c
>>> @@ -161,7 +163,9 @@ static struct rte_eth_conf port_conf = {
>>> .rx_adv_conf = {
>>> .rss_conf = {
>>> .rss_key = NULL,
>>> - .rss_hf = ETH_RSS_IP,
>>> + .rss_hf = ETH_RSS_IP | ETH_RSS_UDP |
>>> + ETH_RSS_TCP | ETH_RSS_SCTP,
>>> +
>>> },
>>> },
>>>
>>>
>>> Why it is necessary to change RSS hash input values?
>>>
>>> As another nit - there are few checkpatch warnings, that probably need
>>> to be addressed.
>>> Apart from that looks good to me.
>>> Thanks
>>> Konstantin
>>>
>>>
>>>
>>
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [RFC 17.05 v1 0/3] Merge l3fwd-acl and l3fwd
2017-03-01 15:29 ` Ravi Kerur
@ 2017-03-04 19:49 ` Ravi Kerur
2017-03-04 20:47 ` Ravi Kerur
0 siblings, 1 reply; 7+ messages in thread
From: Ravi Kerur @ 2017-03-04 19:49 UTC (permalink / raw)
To: Ananyev, Konstantin; +Cc: dev, Richardson, Bruce
Hi Konstantin,
I have sent 'v2' patchset. I need clarifications on following things, if
they should be fixed I will send out 'v3' so please let me know.
Following code changes were done by me manually, not merged.
+++ b/examples/l3fwd/main.c
@@ -161,7 +163,9 @@ static struct rte_eth_conf port_conf = {
.rx_adv_conf = {
.rss_conf = {
.rss_key = NULL,
- .rss_hf = ETH_RSS_IP,
+ .rss_hf = ETH_RSS_IP | ETH_RSS_UDP |
+ ETH_RSS_TCP | ETH_RSS_SCTP,
+
},
The reason I did it is because
LPM/EM has .rss_hf = ETH_RSS_IP
ACL has .rss_hf = ETH_RSS_IP | ETH_RSS_UDP | ETH_RSS_TCP | ETH_RSS_SCTP,
ACL looks like a superset of LPM/EM and functional testing didn't reveal
any issues hence I kept ACL version.
2. Checkpatch errors are all fixed. Some warnings are not fixed and they are
2.a, string length greater than 80 characters
2.b GET_CB_FIELD macro. I could have changed GET_CB_FIELD to inline
function, however, function names cannot be in capital letters. I could
have changed it to 'get_cb_field' inline function, but didn't do it as I
thought it may not be worth the change.
Let me know your inputs.
Thanks.
On Wed, Mar 1, 2017 at 7:29 AM, Ravi Kerur <rkerur@gmail.com> wrote:
> Hi Konstantin,
>
> Thank you for the review.
>
> RSS hash value changes could be due to merge, I didn't make that change. I
> will go through the changes and fix it in 'v2' patch along with RFC removed
> and checkpatch fix.
>
> Thanks.
>
> On Tue, Feb 28, 2017 at 2:36 AM, Ananyev, Konstantin <
> konstantin.ananyev@intel.com> wrote:
>
>> Hi Ravi,
>>
>> >
>> > Thanks to Konstantin and Bruce on first internal review comments. This
>> > patch is RFC for 17.05 to merge l3fwd-acl and l3fwd code and add file
>> > read options to build LPM and EM tables.
>>
>>
>> Thanks for the patch, I think it is really useful one.
>> Can I suggest you re-submit it as non-RFC now, as we are in 17.05 window
>> already?
>> About the patch itself, one question I forgot to ask you before:
>>
>> +++ b/examples/l3fwd/main.c
>> @@ -161,7 +163,9 @@ static struct rte_eth_conf port_conf = {
>> .rx_adv_conf = {
>> .rss_conf = {
>> .rss_key = NULL,
>> - .rss_hf = ETH_RSS_IP,
>> + .rss_hf = ETH_RSS_IP | ETH_RSS_UDP |
>> + ETH_RSS_TCP | ETH_RSS_SCTP,
>> +
>> },
>> },
>>
>>
>> Why it is necessary to change RSS hash input values?
>>
>> As another nit - there are few checkpatch warnings, that probably need
>> to be addressed.
>> Apart from that looks good to me.
>> Thanks
>> Konstantin
>>
>>
>>
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [RFC 17.05 v1 0/3] Merge l3fwd-acl and l3fwd
2017-02-28 10:36 ` Ananyev, Konstantin
@ 2017-03-01 15:29 ` Ravi Kerur
2017-03-04 19:49 ` Ravi Kerur
0 siblings, 1 reply; 7+ messages in thread
From: Ravi Kerur @ 2017-03-01 15:29 UTC (permalink / raw)
To: Ananyev, Konstantin; +Cc: dev, Richardson, Bruce
Hi Konstantin,
Thank you for the review.
RSS hash value changes could be due to merge, I didn't make that change. I
will go through the changes and fix it in 'v2' patch along with RFC removed
and checkpatch fix.
Thanks.
On Tue, Feb 28, 2017 at 2:36 AM, Ananyev, Konstantin <
konstantin.ananyev@intel.com> wrote:
> Hi Ravi,
>
> >
> > Thanks to Konstantin and Bruce on first internal review comments. This
> > patch is RFC for 17.05 to merge l3fwd-acl and l3fwd code and add file
> > read options to build LPM and EM tables.
>
>
> Thanks for the patch, I think it is really useful one.
> Can I suggest you re-submit it as non-RFC now, as we are in 17.05 window
> already?
> About the patch itself, one question I forgot to ask you before:
>
> +++ b/examples/l3fwd/main.c
> @@ -161,7 +163,9 @@ static struct rte_eth_conf port_conf = {
> .rx_adv_conf = {
> .rss_conf = {
> .rss_key = NULL,
> - .rss_hf = ETH_RSS_IP,
> + .rss_hf = ETH_RSS_IP | ETH_RSS_UDP |
> + ETH_RSS_TCP | ETH_RSS_SCTP,
> +
> },
> },
>
>
> Why it is necessary to change RSS hash input values?
>
> As another nit - there are few checkpatch warnings, that probably need
> to be addressed.
> Apart from that looks good to me.
> Thanks
> Konstantin
>
>
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [RFC 17.05 v1 0/3] Merge l3fwd-acl and l3fwd
2017-01-27 20:59 Ravi Kerur
@ 2017-02-28 10:36 ` Ananyev, Konstantin
2017-03-01 15:29 ` Ravi Kerur
0 siblings, 1 reply; 7+ messages in thread
From: Ananyev, Konstantin @ 2017-02-28 10:36 UTC (permalink / raw)
To: Ravi Kerur, dev; +Cc: Richardson, Bruce
Hi Ravi,
>
> Thanks to Konstantin and Bruce on first internal review comments. This
> patch is RFC for 17.05 to merge l3fwd-acl and l3fwd code and add file
> read options to build LPM and EM tables.
Thanks for the patch, I think it is really useful one.
Can I suggest you re-submit it as non-RFC now, as we are in 17.05 window already?
About the patch itself, one question I forgot to ask you before:
+++ b/examples/l3fwd/main.c
@@ -161,7 +163,9 @@ static struct rte_eth_conf port_conf = {
.rx_adv_conf = {
.rss_conf = {
.rss_key = NULL,
- .rss_hf = ETH_RSS_IP,
+ .rss_hf = ETH_RSS_IP | ETH_RSS_UDP |
+ ETH_RSS_TCP | ETH_RSS_SCTP,
+
},
},
Why it is necessary to change RSS hash input values?
As another nit - there are few checkpatch warnings, that probably need
to be addressed.
Apart from that looks good to me.
Thanks
Konstantin
^ permalink raw reply [flat|nested] 7+ messages in thread
* [RFC 17.05 v1 0/3] Merge l3fwd-acl and l3fwd
@ 2017-01-27 20:59 Ravi Kerur
2017-02-28 10:36 ` Ananyev, Konstantin
0 siblings, 1 reply; 7+ messages in thread
From: Ravi Kerur @ 2017-01-27 20:59 UTC (permalink / raw)
To: dev; +Cc: konstantin.ananyev, bruce.richardson, Ravi Kerur
Thanks to Konstantin and Bruce on first internal review comments. This
patch is RFC for 17.05 to merge l3fwd-acl and l3fwd code and add file
read options to build LPM and EM tables.
Ravi Kerur (3):
Merge l3fwd-acl and l3fwd
LPM config file read option
EM config file read option
examples/l3fwd-acl/Makefile | 56 -
examples/l3fwd-acl/main.c | 2079 -------------------------------------
examples/l3fwd/Makefile | 2 +-
examples/l3fwd/l3fwd.h | 92 ++
examples/l3fwd/l3fwd_acl.c | 1029 ++++++++++++++++++
examples/l3fwd/l3fwd_acl.h | 234 +++++
examples/l3fwd/l3fwd_acl_scalar.h | 181 ++++
examples/l3fwd/l3fwd_em.c | 390 +++++--
examples/l3fwd/l3fwd_lpm.c | 322 ++++--
examples/l3fwd/main.c | 231 +++--
10 files changed, 2275 insertions(+), 2341 deletions(-)
delete mode 100644 examples/l3fwd-acl/Makefile
delete mode 100644 examples/l3fwd-acl/main.c
create mode 100644 examples/l3fwd/l3fwd_acl.c
create mode 100644 examples/l3fwd/l3fwd_acl.h
create mode 100644 examples/l3fwd/l3fwd_acl_scalar.h
--
2.7.4
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2017-03-05 19:55 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-03-05 13:15 [RFC 17.05 v1 0/3] Merge l3fwd-acl and l3fwd Ananyev, Konstantin
2017-03-05 19:55 ` Ravi Kerur
-- strict thread matches above, loose matches on Subject: below --
2017-01-27 20:59 Ravi Kerur
2017-02-28 10:36 ` Ananyev, Konstantin
2017-03-01 15:29 ` Ravi Kerur
2017-03-04 19:49 ` Ravi Kerur
2017-03-04 20:47 ` Ravi Kerur
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.