[ovs-dev] [PATCH v2 09/22] datapath: Refactor labels initialization.
Jarno Rajahalme
jarno at ovn.org
Sat Mar 4 00:04:56 UTC 2017
> On Mar 3, 2017, at 1:44 PM, Joe Stringer <joe at ovn.org> wrote:
>
>
>
> On 3/03/2017 10:37, "Jarno Rajahalme" <jarno at ovn.org <mailto:jarno at ovn.org>> wrote:
>
>> On Mar 2, 2017, at 5:26 PM, Joe Stringer <joe at ovn.org <mailto:joe at ovn.org>> wrote:
>>
>> On 28 February 2017 at 17:17, Jarno Rajahalme <jarno at ovn.org <mailto:jarno at ovn.org>> wrote:
>>> Upstream commit:
>>>
>>> Refactoring conntrack labels initialization makes changes in later
>>> patches easier to review.
>>>
>>> Signed-off-by: Jarno Rajahalme <jarno at ovn.org <mailto:jarno at ovn.org>>
>>> Acked-by: Pravin B Shelar <pshelar at ovn.org <mailto:pshelar at ovn.org>>
>>> Acked-by: Joe Stringer <joe at ovn.org <mailto:joe at ovn.org>>
>>> Signed-off-by: David S. Miller <davem at davemloft.net <mailto:davem at davemloft.net>>
>>>
>>> Signed-off-by: Jarno Rajahalme <jarno at ovn.org <mailto:jarno at 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?
>
> When we compile the out of tree module for a particular kernel, this information should be available. I don't think that we try to support compiling against one kernel with one definition of the labels length, then allow that same module to run on another kernel with a different definition. So it should be fine to omit so long as there are still the compile time checks.
>
But my understanding is that the compile time checks only apply to newer kernels, where the available storage for labels is a compile time configuration, rather than a run-time number of words.
Jarno
>
> 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 at openvswitch.org <mailto:dev at openvswitch.org>
>>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev <https://mail.openvswitch.org/mailman/listinfo/ovs-dev>
More information about the dev
mailing list