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 <rke...@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.anan...@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
>>
>>
>>
>
