| Message ID | 1500445102-123532-2-git-send-email-jpettit@ovn.org |
|---|---|
| State | Accepted |
| Headers | show |
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.
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.
Thanks Darrell
On 7/18/17, 11:18 PM, "ovs-dev-bounces@openvswitch.org on behalf of Justin Pettit" <ovs-dev-bounces@openvswitch.org on behalf of jpettit@ovn.org> wrote:
The userspace datapath hardcodes support for the features it supports,
but it was missing "ct_state_nat", "ct_orig_tuple", and "ct_orig_tuple6".
Signed-off-by: Justin Pettit <jpettit@ovn.org>
---
lib/dpif-netdev.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
index 1dd0d63ebddb..3cd0e95eb0a3 100644
--- a/lib/dpif-netdev.c
+++ b/lib/dpif-netdev.c
@@ -112,6 +112,9 @@ static struct odp_support dp_netdev_support = {
.ct_zone = true,
.ct_mark = true,
.ct_label = true,
+ .ct_state_nat = true,
+ .ct_orig_tuple = true,
+ .ct_orig_tuple6 = true,
};
/* Stores a miniflow with inline values */
--
2.7.4
_______________________________________________
dev mailing list
dev@openvswitch.org
https://urldefense.proofpoint.com/v2/url?u=https-3A__mail.openvswitch.org_mailman_listinfo_ovs-2Ddev&d=DwICAg&c=uilaK90D4TOVoH58JNXRgQ&r=BVhFA09CGX7JQ5Ih-uZnsw&m=IB5v8vX446JEBsGI6fRh-BZgavpw4tCmjNae3I_Ow8I&s=teWT8FxDJPD0Q3Zc0MbSAz0S4zGTZiN01Zpio7eJzKk&e=
> On Jul 19, 2017, at 2:45 AM, Darrell Ball <dball@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. > 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. Thanks, --Justin
On 7/19/17, 8:01 AM, "Justin Pettit" <jpettit@ovn.org> wrote: > On Jul 19, 2017, at 2:45 AM, Darrell Ball <dball@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 ? Any additional test coverage you can recommend ? > 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. Thanks, --Justin
> On Jul 19, 2017, at 10:45 AM, Darrell Ball <dball@vmware.com> wrote: > > > > On 7/19/17, 8:01 AM, "Justin Pettit" <jpettit@ovn.org> wrote: > > >> On Jul 19, 2017, at 2:45 AM, Darrell Ball <dball@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? > 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. >> 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. 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
On Wed, Jul 19, 2017 at 12:17 PM, Justin Pettit <jpettit@ovn.org> wrote: > > > On Jul 19, 2017, at 10:45 AM, Darrell Ball <dball@vmware.com> wrote: > > > > > > > > On 7/19/17, 8:01 AM, "Justin Pettit" <jpettit@ovn.org> wrote: > > > > > >> On Jul 19, 2017, at 2:45 AM, Darrell Ball <dball@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 <dlu998@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. > > 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 > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
[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 <dlu998@gmail.com> wrote:
>
>
> On Wed, Jul 19, 2017 at 12:17 PM, Justin Pettit <jpettit@ovn.org> wrote:
>
>>
>> > On Jul 19, 2017, at 10:45 AM, Darrell Ball <dball@vmware.com> wrote:
>> >
>> >
>> >
>> > On 7/19/17, 8:01 AM, "Justin Pettit" <jpettit@ovn.org> wrote:
>> >
>> >
>> >> On Jul 19, 2017, at 2:45 AM, Darrell Ball <dball@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 <dlu998@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
>> dev@openvswitch.org
>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>>
>
>
On 19 July 2017 at 14:11, Darrell Ball <dlu998@gmail.com> wrote: > On Wed, Jul 19, 2017 at 1:18 PM, Darrell Ball <dlu998@gmail.com> wrote: >> On Wed, Jul 19, 2017 at 12:17 PM, Justin Pettit <jpettit@ovn.org> wrote: >>> > On Jul 19, 2017, at 10:45 AM, Darrell Ball <dball@vmware.com> wrote: >>> > On 7/19/17, 8:01 AM, "Justin Pettit" <jpettit@ovn.org> wrote: >>> >> On Jul 19, 2017, at 2:45 AM, Darrell Ball <dball@vmware.com> wrote: >>> >> 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 > /////////////// I think that ct_state_nat was there though, so that bit should probably be backported.
-----Original Message----- From: <ovs-dev-bounces@openvswitch.org> on behalf of Joe Stringer <joe@ovn.org> Date: Wednesday, July 19, 2017 at 2:54 PM To: Darrell Ball <dlu998@gmail.com> Cc: "dev@openvswitch.org" <dev@openvswitch.org> Subject: Re: [ovs-dev] [PATCH 2/2] dpif-netdev: Indicate support for various ct features. On 19 July 2017 at 14:11, Darrell Ball <dlu998@gmail.com> wrote: > On Wed, Jul 19, 2017 at 1:18 PM, Darrell Ball <dlu998@gmail.com> wrote: >> On Wed, Jul 19, 2017 at 12:17 PM, Justin Pettit <jpettit@ovn.org> wrote: >>> > On Jul 19, 2017, at 10:45 AM, Darrell Ball <dball@vmware.com> wrote: >>> > On 7/19/17, 8:01 AM, "Justin Pettit" <jpettit@ovn.org> wrote: >>> >> On Jul 19, 2017, at 2:45 AM, Darrell Ball <dball@vmware.com> wrote: >>> >> 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 > /////////////// I think that ct_state_nat was there though, so that bit should probably be backported. __________________________ [Darrell] NAT is not applicable to 2.7 for Userspace.
> On Jul 19, 2017, at 3:18 PM, Darrell Ball <dlu998@gmail.com> wrote: > > 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.. Okay, I updated the patch as follows and pushed it to master: -=-=-=-=-=-=-=-=- The userspace datapath uses a structure to indicate supported features that affects debug output. This commit updates that structure to indicate that "ct_state_nat", "ct_orig_tuple", and "ct_orig_tuple6" are supported. -=-=-=-=-=-=-=-=- > Otherwise > Acked-by: Darrell Ball <dlu998@gmail.com> Thanks, --Justin
diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c index 1dd0d63ebddb..3cd0e95eb0a3 100644 --- a/lib/dpif-netdev.c +++ b/lib/dpif-netdev.c @@ -112,6 +112,9 @@ static struct odp_support dp_netdev_support = { .ct_zone = true, .ct_mark = true, .ct_label = true, + .ct_state_nat = true, + .ct_orig_tuple = true, + .ct_orig_tuple6 = true, }; /* Stores a miniflow with inline values */
The userspace datapath hardcodes support for the features it supports, but it was missing "ct_state_nat", "ct_orig_tuple", and "ct_orig_tuple6". Signed-off-by: Justin Pettit <jpettit@ovn.org> --- lib/dpif-netdev.c | 3 +++ 1 file changed, 3 insertions(+)