| Message ID | 1488331058-40038-10-git-send-email-jarno@ovn.org |
|---|---|
| State | Superseded |
| Headers | show |
On 28 February 2017 at 17:17, Jarno Rajahalme <jarno@ovn.org> wrote: > Upstream commit: > > Refactoring conntrack labels initialization makes changes in later > patches easier to review. > > Signed-off-by: Jarno Rajahalme <jarno@ovn.org> > Acked-by: Pravin B Shelar <pshelar@ovn.org> > Acked-by: Joe Stringer <joe@ovn.org> > Signed-off-by: David S. Miller <davem@davemloft.net> > > Signed-off-by: Jarno Rajahalme <jarno@ovn.org> > --- > datapath/conntrack.c | 113 ++++++++++++++++++++++++++++++--------------------- > 1 file changed, 66 insertions(+), 47 deletions(-) > > diff --git a/datapath/conntrack.c b/datapath/conntrack.c > index dacf34c..adc4315 100644 > --- a/datapath/conntrack.c > +++ b/datapath/conntrack.c > @@ -243,19 +243,12 @@ int ovs_ct_put_key(const struct sw_flow_key *key, struct sk_buff *skb) > return 0; > } > > -static int ovs_ct_set_mark(struct sk_buff *skb, struct sw_flow_key *key, > +static int ovs_ct_set_mark(struct nf_conn *ct, struct sw_flow_key *key, > u32 ct_mark, u32 mask) > { > #if IS_ENABLED(CONFIG_NF_CONNTRACK_MARK) > - enum ip_conntrack_info ctinfo; > - struct nf_conn *ct; > u32 new_mark; > > - /* The connection could be invalid, in which case set_mark is no-op. */ > - ct = nf_ct_get(skb, &ctinfo); > - if (!ct) > - return 0; > - > new_mark = ct_mark | (ct->mark & ~(mask)); > if (ct->mark != new_mark) { > ct->mark = new_mark; > @@ -270,56 +263,71 @@ static int ovs_ct_set_mark(struct sk_buff *skb, struct sw_flow_key *key, > #endif > } > > -static int ovs_ct_set_labels(struct sk_buff *skb, struct sw_flow_key *key, > - const struct ovs_key_ct_labels *labels, > - const struct ovs_key_ct_labels *mask) > +static struct nf_conn_labels *ovs_ct_get_conn_labels(struct nf_conn *ct) > { > - enum ip_conntrack_info ctinfo; > struct nf_conn_labels *cl; > - struct nf_conn *ct; > - > - /* The connection could be invalid, in which case set_label is no-op.*/ > - ct = nf_ct_get(skb, &ctinfo); > - if (!ct) > - return 0; > > cl = nf_ct_labels_find(ct); > if (!cl) { > nf_ct_labels_ext_add(ct); > cl = nf_ct_labels_find(ct); > } > + if (cl && ovs_ct_get_labels_len(cl) < OVS_CT_LABELS_LEN) > + return NULL; The above two lines were not introduced in the upstream code. Do you intend to introduce them? For my current working tree for review/build/test, I will drop these lines. > + return cl; > +} > + > +/* Initialize labels for a new, yet to be committed conntrack entry. Note that > + * since the new connection is not yet confirmed, and thus no-one else has > + * access to it's labels, we simply write them over. > + */ > +static int ovs_ct_init_labels(struct nf_conn *ct, struct sw_flow_key *key, > + const struct ovs_key_ct_labels *labels, > + const struct ovs_key_ct_labels *mask) > +{ > + struct nf_conn_labels *cl; > + u32 *dst; > + int i; > > - if (!cl || ovs_ct_get_labels_len(cl) < OVS_CT_LABELS_LEN) > + cl = ovs_ct_get_conn_labels(ct); > + if (!cl) > return -ENOSPC; > > - if (nf_ct_is_confirmed(ct)) { > - /* Triggers a change event, which makes sense only for > - * confirmed connections. > - */ > - int err = nf_connlabels_replace(ct, labels->ct_labels_32, > - mask->ct_labels_32, > - OVS_CT_LABELS_LEN_32); > - if (err) > - return err; > - } else { > - u32 *dst = (u32 *)cl->bits; > - const u32 *msk = mask->ct_labels_32; > - const u32 *lbl = labels->ct_labels_32; > - int i; > + dst = (u32 *)cl->bits; > + for (i = 0; i < OVS_CT_LABELS_LEN_32; i++) > + dst[i] = (dst[i] & ~mask->ct_labels_32[i]) | > + (labels->ct_labels_32[i] & mask->ct_labels_32[i]); > > - /* No-one else has access to the non-confirmed entry, copy > - * labels over, keeping any bits we are not explicitly setting. > - */ > - for (i = 0; i < OVS_CT_LABELS_LEN_32; i++) > - dst[i] = (dst[i] & ~msk[i]) | (lbl[i] & msk[i]); > + /* Labels are included in the IPCTNL_MSG_CT_NEW event only if the > + * IPCT_LABEL bit it set in the event cache. > + */ > + nf_conntrack_event_cache(IPCT_LABEL, ct); > > - /* Labels are included in the IPCTNL_MSG_CT_NEW event only if > - * the IPCT_LABEL bit it set in the event cache. > - */ > - nf_conntrack_event_cache(IPCT_LABEL, ct); > - } > + memcpy(&key->ct.labels, cl->bits, OVS_CT_LABELS_LEN); > + > + return 0; > +} > + > +static int ovs_ct_set_labels(struct nf_conn *ct, struct sw_flow_key *key, > + const struct ovs_key_ct_labels *labels, > + const struct ovs_key_ct_labels *mask) > +{ > + struct nf_conn_labels *cl; > + int err; > + > + cl = ovs_ct_get_conn_labels(ct); > + if (!cl) > + return -ENOSPC; > + > + err = nf_connlabels_replace(ct, labels->ct_labels_32, > + mask->ct_labels_32, > + OVS_CT_LABELS_LEN_32); > + if (err) > + return err; > + > + memcpy(&key->ct.labels, cl->bits, OVS_CT_LABELS_LEN); > > - ovs_ct_get_labels(ct, &key->ct.labels); > return 0; > } > > @@ -926,25 +934,36 @@ static int ovs_ct_commit(struct net *net, struct sw_flow_key *key, > const struct ovs_conntrack_info *info, > struct sk_buff *skb) > { > + enum ip_conntrack_info ctinfo; > + struct nf_conn *ct; > int err; > > err = __ovs_ct_lookup(net, key, info, skb); > if (err) > return err; > > + /* The connection could be invalid, in which case this is a no-op.*/ > + ct = nf_ct_get(skb, &ctinfo); > + if (!ct) > + return 0; > + > /* Apply changes before confirming the connection so that the initial > * conntrack NEW netlink event carries the values given in the CT > * action. > */ > if (info->mark.mask) { > - err = ovs_ct_set_mark(skb, key, info->mark.value, > + err = ovs_ct_set_mark(ct, key, info->mark.value, > info->mark.mask); > if (err) > return err; > } > if (labels_nonzero(&info->labels.mask)) { > - err = ovs_ct_set_labels(skb, key, &info->labels.value, > - &info->labels.mask); > + if (!nf_ct_is_confirmed(ct)) > + err = ovs_ct_init_labels(ct, key, &info->labels.value, > + &info->labels.mask); > + else > + err = ovs_ct_set_labels(ct, key, &info->labels.value, > + &info->labels.mask); > if (err) > return err; > } > -- > 2.1.4 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> On Mar 2, 2017, at 5:26 PM, Joe Stringer <joe@ovn.org> wrote: > > On 28 February 2017 at 17:17, Jarno Rajahalme <jarno@ovn.org <mailto:jarno@ovn.org>> wrote: >> Upstream commit: >> >> Refactoring conntrack labels initialization makes changes in later >> patches easier to review. >> >> Signed-off-by: Jarno Rajahalme <jarno@ovn.org> >> Acked-by: Pravin B Shelar <pshelar@ovn.org> >> Acked-by: Joe Stringer <joe@ovn.org> >> Signed-off-by: David S. Miller <davem@davemloft.net> >> >> Signed-off-by: Jarno Rajahalme <jarno@ovn.org> >> --- >> datapath/conntrack.c | 113 ++++++++++++++++++++++++++++++--------------------- >> 1 file changed, 66 insertions(+), 47 deletions(-) >> >> diff --git a/datapath/conntrack.c b/datapath/conntrack.c >> index dacf34c..adc4315 100644 >> --- a/datapath/conntrack.c >> +++ b/datapath/conntrack.c >> @@ -243,19 +243,12 @@ int ovs_ct_put_key(const struct sw_flow_key *key, struct sk_buff *skb) >> return 0; >> } >> >> -static int ovs_ct_set_mark(struct sk_buff *skb, struct sw_flow_key *key, >> +static int ovs_ct_set_mark(struct nf_conn *ct, struct sw_flow_key *key, >> u32 ct_mark, u32 mask) >> { >> #if IS_ENABLED(CONFIG_NF_CONNTRACK_MARK) >> - enum ip_conntrack_info ctinfo; >> - struct nf_conn *ct; >> u32 new_mark; >> >> - /* The connection could be invalid, in which case set_mark is no-op. */ >> - ct = nf_ct_get(skb, &ctinfo); >> - if (!ct) >> - return 0; >> - >> new_mark = ct_mark | (ct->mark & ~(mask)); >> if (ct->mark != new_mark) { >> ct->mark = new_mark; >> @@ -270,56 +263,71 @@ static int ovs_ct_set_mark(struct sk_buff *skb, struct sw_flow_key *key, >> #endif >> } >> >> -static int ovs_ct_set_labels(struct sk_buff *skb, struct sw_flow_key *key, >> - const struct ovs_key_ct_labels *labels, >> - const struct ovs_key_ct_labels *mask) >> +static struct nf_conn_labels *ovs_ct_get_conn_labels(struct nf_conn *ct) >> { >> - enum ip_conntrack_info ctinfo; >> struct nf_conn_labels *cl; >> - struct nf_conn *ct; >> - >> - /* The connection could be invalid, in which case set_label is no-op.*/ >> - ct = nf_ct_get(skb, &ctinfo); >> - if (!ct) >> - return 0; >> >> cl = nf_ct_labels_find(ct); >> if (!cl) { >> nf_ct_labels_ext_add(ct); >> cl = nf_ct_labels_find(ct); >> } >> + if (cl && ovs_ct_get_labels_len(cl) < OVS_CT_LABELS_LEN) >> + return NULL; > > The above two lines were not introduced in the upstream code. Do you > intend to introduce them? > Should have mentioned this in a commit message or in a comment. The inclusion of this test is intentional, and the rationale is that it might be possible that the kernel is configured with too little space for labels. However, it is possible that the way OVS kernel module initializes the number of words in labels for older kernels already takes care of this, do you have a take on this? Jarno > For my current working tree for review/build/test, I will drop these lines. > >> + return cl; >> +} >> + >> +/* Initialize labels for a new, yet to be committed conntrack entry. Note that >> + * since the new connection is not yet confirmed, and thus no-one else has >> + * access to it's labels, we simply write them over. >> + */ >> +static int ovs_ct_init_labels(struct nf_conn *ct, struct sw_flow_key *key, >> + const struct ovs_key_ct_labels *labels, >> + const struct ovs_key_ct_labels *mask) >> +{ >> + struct nf_conn_labels *cl; >> + u32 *dst; >> + int i; >> >> - if (!cl || ovs_ct_get_labels_len(cl) < OVS_CT_LABELS_LEN) >> + cl = ovs_ct_get_conn_labels(ct); >> + if (!cl) >> return -ENOSPC; >> >> - if (nf_ct_is_confirmed(ct)) { >> - /* Triggers a change event, which makes sense only for >> - * confirmed connections. >> - */ >> - int err = nf_connlabels_replace(ct, labels->ct_labels_32, >> - mask->ct_labels_32, >> - OVS_CT_LABELS_LEN_32); >> - if (err) >> - return err; >> - } else { >> - u32 *dst = (u32 *)cl->bits; >> - const u32 *msk = mask->ct_labels_32; >> - const u32 *lbl = labels->ct_labels_32; >> - int i; >> + dst = (u32 *)cl->bits; >> + for (i = 0; i < OVS_CT_LABELS_LEN_32; i++) >> + dst[i] = (dst[i] & ~mask->ct_labels_32[i]) | >> + (labels->ct_labels_32[i] & mask->ct_labels_32[i]); >> >> - /* No-one else has access to the non-confirmed entry, copy >> - * labels over, keeping any bits we are not explicitly setting. >> - */ >> - for (i = 0; i < OVS_CT_LABELS_LEN_32; i++) >> - dst[i] = (dst[i] & ~msk[i]) | (lbl[i] & msk[i]); >> + /* Labels are included in the IPCTNL_MSG_CT_NEW event only if the >> + * IPCT_LABEL bit it set in the event cache. >> + */ >> + nf_conntrack_event_cache(IPCT_LABEL, ct); >> >> - /* Labels are included in the IPCTNL_MSG_CT_NEW event only if >> - * the IPCT_LABEL bit it set in the event cache. >> - */ >> - nf_conntrack_event_cache(IPCT_LABEL, ct); >> - } >> + memcpy(&key->ct.labels, cl->bits, OVS_CT_LABELS_LEN); >> + >> + return 0; >> +} >> + >> +static int ovs_ct_set_labels(struct nf_conn *ct, struct sw_flow_key *key, >> + const struct ovs_key_ct_labels *labels, >> + const struct ovs_key_ct_labels *mask) >> +{ >> + struct nf_conn_labels *cl; >> + int err; >> + >> + cl = ovs_ct_get_conn_labels(ct); >> + if (!cl) >> + return -ENOSPC; >> + >> + err = nf_connlabels_replace(ct, labels->ct_labels_32, >> + mask->ct_labels_32, >> + OVS_CT_LABELS_LEN_32); >> + if (err) >> + return err; >> + >> + memcpy(&key->ct.labels, cl->bits, OVS_CT_LABELS_LEN); >> >> - ovs_ct_get_labels(ct, &key->ct.labels); >> return 0; >> } >> >> @@ -926,25 +934,36 @@ static int ovs_ct_commit(struct net *net, struct sw_flow_key *key, >> const struct ovs_conntrack_info *info, >> struct sk_buff *skb) >> { >> + enum ip_conntrack_info ctinfo; >> + struct nf_conn *ct; >> int err; >> >> err = __ovs_ct_lookup(net, key, info, skb); >> if (err) >> return err; >> >> + /* The connection could be invalid, in which case this is a no-op.*/ >> + ct = nf_ct_get(skb, &ctinfo); >> + if (!ct) >> + return 0; >> + >> /* Apply changes before confirming the connection so that the initial >> * conntrack NEW netlink event carries the values given in the CT >> * action. >> */ >> if (info->mark.mask) { >> - err = ovs_ct_set_mark(skb, key, info->mark.value, >> + err = ovs_ct_set_mark(ct, key, info->mark.value, >> info->mark.mask); >> if (err) >> return err; >> } >> if (labels_nonzero(&info->labels.mask)) { >> - err = ovs_ct_set_labels(skb, key, &info->labels.value, >> - &info->labels.mask); >> + if (!nf_ct_is_confirmed(ct)) >> + err = ovs_ct_init_labels(ct, key, &info->labels.value, >> + &info->labels.mask); >> + else >> + err = ovs_ct_set_labels(ct, key, &info->labels.value, >> + &info->labels.mask); >> if (err) >> return err; >> } >> -- >> 2.1.4 >> >> _______________________________________________ >> dev mailing list >> dev@openvswitch.org <mailto:dev@openvswitch.org> >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev <https://mail.openvswitch.org/mailman/listinfo/ovs-dev>
diff --git a/datapath/conntrack.c b/datapath/conntrack.c index dacf34c..adc4315 100644 --- a/datapath/conntrack.c +++ b/datapath/conntrack.c @@ -243,19 +243,12 @@ int ovs_ct_put_key(const struct sw_flow_key *key, struct sk_buff *skb) return 0; } -static int ovs_ct_set_mark(struct sk_buff *skb, struct sw_flow_key *key, +static int ovs_ct_set_mark(struct nf_conn *ct, struct sw_flow_key *key, u32 ct_mark, u32 mask) { #if IS_ENABLED(CONFIG_NF_CONNTRACK_MARK) - enum ip_conntrack_info ctinfo; - struct nf_conn *ct; u32 new_mark; - /* The connection could be invalid, in which case set_mark is no-op. */ - ct = nf_ct_get(skb, &ctinfo); - if (!ct) - return 0; - new_mark = ct_mark | (ct->mark & ~(mask)); if (ct->mark != new_mark) { ct->mark = new_mark; @@ -270,56 +263,71 @@ static int ovs_ct_set_mark(struct sk_buff *skb, struct sw_flow_key *key, #endif } -static int ovs_ct_set_labels(struct sk_buff *skb, struct sw_flow_key *key, - const struct ovs_key_ct_labels *labels, - const struct ovs_key_ct_labels *mask) +static struct nf_conn_labels *ovs_ct_get_conn_labels(struct nf_conn *ct) { - enum ip_conntrack_info ctinfo; struct nf_conn_labels *cl; - struct nf_conn *ct; - - /* The connection could be invalid, in which case set_label is no-op.*/ - ct = nf_ct_get(skb, &ctinfo); - if (!ct) - return 0; cl = nf_ct_labels_find(ct); if (!cl) { nf_ct_labels_ext_add(ct); cl = nf_ct_labels_find(ct); } + if (cl && ovs_ct_get_labels_len(cl) < OVS_CT_LABELS_LEN) + return NULL; + + return cl; +} + +/* Initialize labels for a new, yet to be committed conntrack entry. Note that + * since the new connection is not yet confirmed, and thus no-one else has + * access to it's labels, we simply write them over. + */ +static int ovs_ct_init_labels(struct nf_conn *ct, struct sw_flow_key *key, + const struct ovs_key_ct_labels *labels, + const struct ovs_key_ct_labels *mask) +{ + struct nf_conn_labels *cl; + u32 *dst; + int i; - if (!cl || ovs_ct_get_labels_len(cl) < OVS_CT_LABELS_LEN) + cl = ovs_ct_get_conn_labels(ct); + if (!cl) return -ENOSPC; - if (nf_ct_is_confirmed(ct)) { - /* Triggers a change event, which makes sense only for - * confirmed connections. - */ - int err = nf_connlabels_replace(ct, labels->ct_labels_32, - mask->ct_labels_32, - OVS_CT_LABELS_LEN_32); - if (err) - return err; - } else { - u32 *dst = (u32 *)cl->bits; - const u32 *msk = mask->ct_labels_32; - const u32 *lbl = labels->ct_labels_32; - int i; + dst = (u32 *)cl->bits; + for (i = 0; i < OVS_CT_LABELS_LEN_32; i++) + dst[i] = (dst[i] & ~mask->ct_labels_32[i]) | + (labels->ct_labels_32[i] & mask->ct_labels_32[i]); - /* No-one else has access to the non-confirmed entry, copy - * labels over, keeping any bits we are not explicitly setting. - */ - for (i = 0; i < OVS_CT_LABELS_LEN_32; i++) - dst[i] = (dst[i] & ~msk[i]) | (lbl[i] & msk[i]); + /* Labels are included in the IPCTNL_MSG_CT_NEW event only if the + * IPCT_LABEL bit it set in the event cache. + */ + nf_conntrack_event_cache(IPCT_LABEL, ct); - /* Labels are included in the IPCTNL_MSG_CT_NEW event only if - * the IPCT_LABEL bit it set in the event cache. - */ - nf_conntrack_event_cache(IPCT_LABEL, ct); - } + memcpy(&key->ct.labels, cl->bits, OVS_CT_LABELS_LEN); + + return 0; +} + +static int ovs_ct_set_labels(struct nf_conn *ct, struct sw_flow_key *key, + const struct ovs_key_ct_labels *labels, + const struct ovs_key_ct_labels *mask) +{ + struct nf_conn_labels *cl; + int err; + + cl = ovs_ct_get_conn_labels(ct); + if (!cl) + return -ENOSPC; + + err = nf_connlabels_replace(ct, labels->ct_labels_32, + mask->ct_labels_32, + OVS_CT_LABELS_LEN_32); + if (err) + return err; + + memcpy(&key->ct.labels, cl->bits, OVS_CT_LABELS_LEN); - ovs_ct_get_labels(ct, &key->ct.labels); return 0; } @@ -926,25 +934,36 @@ static int ovs_ct_commit(struct net *net, struct sw_flow_key *key, const struct ovs_conntrack_info *info, struct sk_buff *skb) { + enum ip_conntrack_info ctinfo; + struct nf_conn *ct; int err; err = __ovs_ct_lookup(net, key, info, skb); if (err) return err; + /* The connection could be invalid, in which case this is a no-op.*/ + ct = nf_ct_get(skb, &ctinfo); + if (!ct) + return 0; + /* Apply changes before confirming the connection so that the initial * conntrack NEW netlink event carries the values given in the CT * action. */ if (info->mark.mask) { - err = ovs_ct_set_mark(skb, key, info->mark.value, + err = ovs_ct_set_mark(ct, key, info->mark.value, info->mark.mask); if (err) return err; } if (labels_nonzero(&info->labels.mask)) { - err = ovs_ct_set_labels(skb, key, &info->labels.value, - &info->labels.mask); + if (!nf_ct_is_confirmed(ct)) + err = ovs_ct_init_labels(ct, key, &info->labels.value, + &info->labels.mask); + else + err = ovs_ct_set_labels(ct, key, &info->labels.value, + &info->labels.mask); if (err) return err; }
Upstream commit: Refactoring conntrack labels initialization makes changes in later patches easier to review. Signed-off-by: Jarno Rajahalme <jarno@ovn.org> Acked-by: Pravin B Shelar <pshelar@ovn.org> Acked-by: Joe Stringer <joe@ovn.org> Signed-off-by: David S. Miller <davem@davemloft.net> Signed-off-by: Jarno Rajahalme <jarno@ovn.org> --- datapath/conntrack.c | 113 ++++++++++++++++++++++++++++++--------------------- 1 file changed, 66 insertions(+), 47 deletions(-)