[Darrell] Backporting may not apply here as none of Jarno's original tuple
code made it
to 2.7.
On Wed, Jul 19, 2017 at 1:18 PM, Darrell Ball <dlu...@gmail.com> wrote:
>
>
> On Wed, Jul 19, 2017 at 12:17 PM, Justin Pettit <jpet...@ovn.org> wrote:
>
>>
>> > On Jul 19, 2017, at 10:45 AM, Darrell Ball <db...@vmware.com> wrote:
>> >
>> >
>> >
>> > On 7/19/17, 8:01 AM, "Justin Pettit" <jpet...@ovn.org> wrote:
>> >
>> >
>> >> On Jul 19, 2017, at 2:45 AM, Darrell Ball <db...@vmware.com> wrote:
>> >>
>> >> This change does not seem to be all that useful.
>> >>
>> >> When rules are constructed, mask and action support do check
>> previously probed support
>> >> which will be ‘TRUE’.
>> >>
>> >> Another way to see that the below settings are not useful is to set
>> everything to ‘false’ (see below) and run all
>> >> the system tests in userspace, which will all pass.
>> >
>> > This change does affect behavior. For example, upcall debug log
>> messages will now show ct_orig_tuple members. The reason
>> > is that dp_netdev_upcall() passes those support parameters to
>> odp_flow_key_from_flow(), which thinks that the datapath
>> > doesn't support those features, so it doesn't print the values.
>> >
>> > I see, so you noticed it affected debug output.
>> > I see the hardcoded values passed in 3 functions.
>> > Was there any other impact you can foresee ?
>>
>> I don't. Do you?
>>
>
> [Darrell]
> I could not see any; it is good the impact from this hybrid support thing
> was limited
> to debug o/p then.
> I don't see any new issues from this patch addition, and it is the only
> reasonable
> option for backporting.
>
> In your original commit message, you said:
>
> "The userspace datapath hardcodes support for the features it supports,
> but it was missing "ct_state_nat", "ct_orig_tuple", and "ct_orig_tuple6"."
>
> Can you modify that so that it reflects that it is partially true and the
> impact
> we now know/assume is limited to debug o/p per your confirmation..
>
> Otherwise
> Acked-by: Darrell Ball <dlu...@gmail.com>
>
>
> ////////////////
>
>
>
>> > Any additional test coverage you can recommend ?
>>
>> I don't think so, since likely future issues will come from adding new
>> support features and not populating this structure--not from this reverting
>> somehow.
>>
>
> [Darrell]
> JTBC, I don't think populating this structure per this patch will cause
> any additional issues.
>
> I was more concerned that if there was any existing issues besides debug
> o/p,
> that we might not have full test coverage. But since we don't think that
> is the case, the
> additional testing is not needed.
> ////////////////
>
>
>
>
>>
>> >> By the way, the .max_vlan_headers and .max_mpls_headers fields, which
>> I did not change are pretty big
>> >> numbers and I am fairly sure OVS does not really support that many
>> vlans and labels.
>> >>
>> >> static struct odp_support dp_netdev_support = {
>> >> .max_vlan_headers = SIZE_MAX,
>> >> .max_mpls_depth = SIZE_MAX,
>> >> .recirc = false,
>> >> .ct_state = false,
>> >> .ct_zone = false,
>> >> .ct_mark = false,
>> >> .ct_label = false,
>> >> .ct_state_nat = false,
>> >> .ct_orig_tuple = false,
>> >> .ct_orig_tuple6 = false,
>> >> };
>> >>
>> >> I think it may be better to clean this up. I can do this if you are ok
>> with that; either way is fine with me.
>> >
>> > I agree that since the datapath features are probed, we should pass
>> those parameters around instead of using these hardcoded values. However,
>> it was a more invasive change than I wanted to make at the time, and I'd
>> want to apply this fix regardless. I was going to add using the probed
>> values to my to-do list, but I'm happy if you want to take it.
>> >
>> > If there is a pressing reason to make a temporary fix, then that is
>> fine.
>>
>> This seems like an obvious fix, and can be safely backported to earlier
>> version. I agree that feature-probing is a better long-term solution, but
>> I'd prefer to backport this rather than a bigger change. I would have
>> proposed this patch anyway.
>>
>
[Darrell] Backporting may not apply here as none of Jarno's original tuple
code made it
to 2.7
///////////////
>
>> By the way, can you look at adjusting your email client to quote replies
>> instead of merely indenting them? It makes it hard to see what the other
>> person was saying when you reply.
>
>
>> --Justin
>>
>>
>> _______________________________________________
>> dev mailing list
>> d...@openvswitch.org
>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>>
>
>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
