| Message ID | 07b27739e79ed3d357328526d75aa5129aa1107f.1600704955.git.lorenzo.bianconi@redhat.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | fix localnet QoS configuration after I-P | expand |
On Mon, Sep 21, 2020 at 9:56 PM Lorenzo Bianconi < lorenzo.bianconi@redhat.com> wrote: > build_local_bindings() takes care of local binding only so if the > controller does not have any active tunnel egress_ifaces will be empty > and qos_map map will not be populated. Rely on qos_map directly for > localnet port in consider_localnet_lport() > > Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com> > Hi Lorenzo, The patch LGTM. I think we can apply this patch. But I find the commit message to be confusing. I guess you want to say that use qos_map instead of 'qos_map_ptr' for consider_localnet_lport() since qos_map_ptr may be NULL if there are no active tunnels. Can you please rephrase the commit message and share it here ? Or see if the below message is fine ? ****** binding: rely on qos_map for consider_localnet_lport Use hmap 'qos_map' directly in consider_localnet_lport() instead of 'qos_map_ptr' since 'qos_map_ptr' could be NULL if there are no active tunnel interfaces. ***** Thanks Numan --- > controller/binding.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/controller/binding.c b/controller/binding.c > index 0397e145a..107bc28a6 100644 > --- a/controller/binding.c > +++ b/controller/binding.c > @@ -1444,7 +1444,7 @@ binding_run(struct binding_ctx_in *b_ctx_in, struct > binding_ctx_out *b_ctx_out) > break; > > case LP_LOCALNET: { > - consider_localnet_lport(pb, b_ctx_in, b_ctx_out, qos_map_ptr); > + consider_localnet_lport(pb, b_ctx_in, b_ctx_out, &qos_map); > struct localnet_lport *lnet_lport = xmalloc(sizeof > *lnet_lport); > lnet_lport->pb = pb; > ovs_list_push_back(&localnet_lports, &lnet_lport->list_node); > -- > 2.26.2 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > >
> On Mon, Sep 21, 2020 at 9:56 PM Lorenzo Bianconi < > lorenzo.bianconi@redhat.com> wrote: > > > build_local_bindings() takes care of local binding only so if the > > controller does not have any active tunnel egress_ifaces will be empty > > and qos_map map will not be populated. Rely on qos_map directly for > > localnet port in consider_localnet_lport() > > > > Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com> > > > > Hi Lorenzo, > > The patch LGTM. I think we can apply this patch. But I find the commit > message to be confusing. > > I guess you want to say that use qos_map instead of 'qos_map_ptr' > for consider_localnet_lport() > since qos_map_ptr may be NULL if there are no active tunnels. > > Can you please rephrase the commit message and share it here ? > > Or see if the below message is fine ? Hi Numan, thx for the review. Yes, right. I will repost this with a proper commit message. Regards, Lorenzo > > ****** > binding: rely on qos_map for consider_localnet_lport > > Use hmap 'qos_map' directly in consider_localnet_lport() instead of > 'qos_map_ptr' > since 'qos_map_ptr' could be NULL if there are no active tunnel > interfaces. > ***** > > Thanks > Numan > > --- > > controller/binding.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/controller/binding.c b/controller/binding.c > > index 0397e145a..107bc28a6 100644 > > --- a/controller/binding.c > > +++ b/controller/binding.c > > @@ -1444,7 +1444,7 @@ binding_run(struct binding_ctx_in *b_ctx_in, struct > > binding_ctx_out *b_ctx_out) > > break; > > > > case LP_LOCALNET: { > > - consider_localnet_lport(pb, b_ctx_in, b_ctx_out, qos_map_ptr); > > + consider_localnet_lport(pb, b_ctx_in, b_ctx_out, &qos_map); > > struct localnet_lport *lnet_lport = xmalloc(sizeof > > *lnet_lport); > > lnet_lport->pb = pb; > > ovs_list_push_back(&localnet_lports, &lnet_lport->list_node); > > -- > > 2.26.2 > > > > _______________________________________________ > > dev mailing list > > dev@openvswitch.org > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > > >
> On Mon, Sep 21, 2020 at 9:56 PM Lorenzo Bianconi < > lorenzo.bianconi@redhat.com> wrote: > > > build_local_bindings() takes care of local binding only so if the > > controller does not have any active tunnel egress_ifaces will be empty > > and qos_map map will not be populated. Rely on qos_map directly for > > localnet port in consider_localnet_lport() > > > > Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com> > > > > Hi Lorenzo, > > The patch LGTM. I think we can apply this patch. But I find the commit > message to be confusing. > > I guess you want to say that use qos_map instead of 'qos_map_ptr' > for consider_localnet_lport() > since qos_map_ptr may be NULL if there are no active tunnels. > > Can you please rephrase the commit message and share it here ? > > Or see if the below message is fine ? > > ****** > binding: rely on qos_map for consider_localnet_lport > > Use hmap 'qos_map' directly in consider_localnet_lport() instead of > 'qos_map_ptr' > since 'qos_map_ptr' could be NULL if there are no active tunnel > interfaces. > ***** Thx Numan, this commit message is fine to me. Regards, Lorenzo > > Thanks > Numan > > --- > > controller/binding.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/controller/binding.c b/controller/binding.c > > index 0397e145a..107bc28a6 100644 > > --- a/controller/binding.c > > +++ b/controller/binding.c > > @@ -1444,7 +1444,7 @@ binding_run(struct binding_ctx_in *b_ctx_in, struct > > binding_ctx_out *b_ctx_out) > > break; > > > > case LP_LOCALNET: { > > - consider_localnet_lport(pb, b_ctx_in, b_ctx_out, qos_map_ptr); > > + consider_localnet_lport(pb, b_ctx_in, b_ctx_out, &qos_map); > > struct localnet_lport *lnet_lport = xmalloc(sizeof > > *lnet_lport); > > lnet_lport->pb = pb; > > ovs_list_push_back(&localnet_lports, &lnet_lport->list_node); > > -- > > 2.26.2 > > > > _______________________________________________ > > dev mailing list > > dev@openvswitch.org > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > > >
On Wed, Sep 23, 2020 at 2:01 PM Lorenzo Bianconi < lorenzo.bianconi@redhat.com> wrote: > > On Mon, Sep 21, 2020 at 9:56 PM Lorenzo Bianconi < > > lorenzo.bianconi@redhat.com> wrote: > > > > > build_local_bindings() takes care of local binding only so if the > > > controller does not have any active tunnel egress_ifaces will be empty > > > and qos_map map will not be populated. Rely on qos_map directly for > > > localnet port in consider_localnet_lport() > > > > > > Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com> > > > > > > > Hi Lorenzo, > > > > The patch LGTM. I think we can apply this patch. But I find the commit > > message to be confusing. > > > > I guess you want to say that use qos_map instead of 'qos_map_ptr' > > for consider_localnet_lport() > > since qos_map_ptr may be NULL if there are no active tunnels. > > > > Can you please rephrase the commit message and share it here ? > > > > Or see if the below message is fine ? > > > > ****** > > binding: rely on qos_map for consider_localnet_lport > > > > Use hmap 'qos_map' directly in consider_localnet_lport() instead of > > 'qos_map_ptr' > > since 'qos_map_ptr' could be NULL if there are no active tunnel > > interfaces. > > ***** > > Thx Numan, this commit message is fine to me. > Thanks Lorenzo. I applied this patch to master, branch-20.09 and 20.06. Numan > > Regards, > Lorenzo > > > > > Thanks > > Numan > > > > --- > > > controller/binding.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/controller/binding.c b/controller/binding.c > > > index 0397e145a..107bc28a6 100644 > > > --- a/controller/binding.c > > > +++ b/controller/binding.c > > > @@ -1444,7 +1444,7 @@ binding_run(struct binding_ctx_in *b_ctx_in, > struct > > > binding_ctx_out *b_ctx_out) > > > break; > > > > > > case LP_LOCALNET: { > > > - consider_localnet_lport(pb, b_ctx_in, b_ctx_out, > qos_map_ptr); > > > + consider_localnet_lport(pb, b_ctx_in, b_ctx_out, > &qos_map); > > > struct localnet_lport *lnet_lport = xmalloc(sizeof > > > *lnet_lport); > > > lnet_lport->pb = pb; > > > ovs_list_push_back(&localnet_lports, > &lnet_lport->list_node); > > > -- > > > 2.26.2 > > > > > > _______________________________________________ > > > dev mailing list > > > dev@openvswitch.org > > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > > > > > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > >
diff --git a/controller/binding.c b/controller/binding.c index 0397e145a..107bc28a6 100644 --- a/controller/binding.c +++ b/controller/binding.c @@ -1444,7 +1444,7 @@ binding_run(struct binding_ctx_in *b_ctx_in, struct binding_ctx_out *b_ctx_out) break; case LP_LOCALNET: { - consider_localnet_lport(pb, b_ctx_in, b_ctx_out, qos_map_ptr); + consider_localnet_lport(pb, b_ctx_in, b_ctx_out, &qos_map); struct localnet_lport *lnet_lport = xmalloc(sizeof *lnet_lport); lnet_lport->pb = pb; ovs_list_push_back(&localnet_lports, &lnet_lport->list_node);
build_local_bindings() takes care of local binding only so if the controller does not have any active tunnel egress_ifaces will be empty and qos_map map will not be populated. Rely on qos_map directly for localnet port in consider_localnet_lport() Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com> --- controller/binding.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)