On Oct 1, 2013, at 3:07 PM, Jarno Rajahalme wrote:
>
> On Sep 20, 2013, at 1:04 AM, pritesh <pritesh.koth...@cisco.com> wrote:
>
>> Support for nsh service index (nsi) is added, mainly incoming
>> nsi in a flow can be matched and appropriate action can be
>> taken on the flow based on it.
>>
>> Signed-off-by: pritesh <pritesh.koth...@cisco.com>
>>
>>
> ...
>> diff --git a/lib/meta-flow.c b/lib/meta-flow.c
>> index 67ddd3e..8e11de0 100644
>> --- a/lib/meta-flow.c
>> +++ b/lib/meta-flow.c
>> @@ -711,6 +711,17 @@ static const struct mf_field mf_fields[MFF_N_IDS] = {
>> 0, NULL,
>> OFPUTIL_P_OF10_NXM_ANY,
>> OFPUTIL_P_OF10_NXM_ANY,
>> + }, {
>> + MFF_NSI, "nsi", NULL,
>> + MF_FIELD_SIZES(u8),
>> + MFM_FULLY,
>
> Does it make sense for the NSI to be fully maskable, as it is supposed to be
> decremented on each hop? Other TTL-type fields are not maskable, see
> "tun_ttl" and "nw_ttl".
there are few minor cases where it might be useful, but yes will change these
to be consistent with other ttl's.
>
>> + MFS_HEXADECIMAL,
>
> Even though it does not make any difference in practice, currently, this
> should probably be MFS_DECIMAL.
>
>> + MFP_NONE,
>> + false,
>> + 0, NULL,
>> + 0, NULL,
>> + OFPUTIL_P_OF10_NXM_ANY,
>> + OFPUTIL_P_OF10_NXM_ANY,
>
> These should be:
>
> OFPUTIL_P_NONE,
> OFPUTIL_P_NONE,
>
>
> As matching of nsi is not yet enabled.
same here, will take care of changes here.
>
>>
> …
>
>> static int
>> set_tunnel_config(struct netdev *dev_, const struct smap *args)
>> {
>> @@ -456,6 +485,10 @@ set_tunnel_config(struct netdev *dev_, const struct
>> smap *args)
>> !strcmp(node->key, "in_nsp") ||
>> !strcmp(node->key, "out_nsp")) {
>> /* Handled separately below. */
>> + } else if (!strcmp(node->key, "nsi") ||
>> + !strcmp(node->key, "in_nsi") ||
>> + !strcmp(node->key, "out_nsi")) {
>> + /* Handled separately below. */
>> } else {
>> VLOG_WARN("%s: unknown %s argument '%s'", name, type, node->key);
>> }
>> @@ -532,6 +565,24 @@ set_tunnel_config(struct netdev *dev_, const struct
>> smap *args)
>> &tnl_cfg.out_nsp_present,
>> &tnl_cfg.out_nsp_flow);
>>
>> + tnl_cfg.in_nsi = parse_nsi(args, "in_nsi",
>> + &tnl_cfg.in_nsi_present,
>> + &tnl_cfg.in_nsi_flow);
>> +
>> + tnl_cfg.out_nsi = parse_nsi(args, "out_nsi",
>> + &tnl_cfg.out_nsi_present,
>> + &tnl_cfg.out_nsi_flow);
>> +
>> + if (tnl_cfg.dst_port == htons(NSH_DST_PORT)) {
>
> Could you use any "nsp" setting to detect the need for default NSIs instead?
yep actually this topic was also handled in another email thread, just to be be
clear
i will try to detect nsh on incoming flows and process those instead of relying
on
port number here.
>
>> + /* Default nsh service index is 1, if lower packet is dropped */
>> + if (!tnl_cfg.in_nsi) {
>> + tnl_cfg.in_nsi = 1;
>> + }
>> + if (!tnl_cfg.out_nsi) {
>> + tnl_cfg.out_nsi = 1;
>> + }
>> + }
>> +
>> ovs_mutex_lock(&dev->mutex);
>> dev->tnl_cfg = tnl_cfg;
>> netdev_vport_poll_notify(dev);
>> diff --git a/lib/netdev.h b/lib/netdev.h
>> index 36817f5..c23fe25 100644
>> --- a/lib/netdev.h
>> +++ b/lib/netdev.h
>> @@ -110,6 +110,14 @@ struct netdev_tunnel_config {
>> bool out_nsp_flow;
>> ovs_be32 out_nsp; /* outgoing NSH service path */
>>
>> + bool in_nsi_present;
>> + bool in_nsi_flow;
>> + uint8_t in_nsi; /* incoming NSH service index */
>> +
>> + bool out_nsi_present;
>> + bool out_nsi_flow;
>> + uint8_t out_nsi; /* outgoing NSH service index */
>> +
>
> Is the intention that the NSI decrementing happens via explicit matching,
> e.g., match on nsi=5, and in action set nsi to 4? Or, if the tunnel is
> configured
> for incoming nsi 5, then each action must contain an action setting nsi to a
> lower value, e.g., 4?
nice one, actually both should happen, but i see that i have handled none, i
will try to take care of this,
though i am not sure how action setting works for incoming matching to outgoing
flows,
any hint how to go about that would be great.
>
>> bool in_key_present;
>> bool in_key_flow;
>> ovs_be64 in_key;
>>
> ...
>> @@ -428,22 +437,35 @@ tnl_find(const struct flow *flow)
>> OVS_REQ_RDLOCK(rwlock)
>> bool in_key_flow;
>> bool ip_dst_flow;
>> bool in_nsp_flow;
>> + bool in_nsi_flow;
>> enum ip_src_type ip_src;
>> };
>>
>> static const struct tnl_match_pattern patterns[] = {
>> - { false, false, false, IP_SRC_EXACT }, /* remote_ip, local_ip,
>> in_key. */
>> - { false, false, false, IP_SRC_ANY }, /* remote_ip, in_key. */
>> - { true, false, false, IP_SRC_EXACT }, /* remote_ip, local_ip. */
>> - { true, false, false, IP_SRC_ANY }, /* remote_ip. */
>> - { true, true, false, IP_SRC_ANY }, /* Flow-based remote. */
>> - { true, true, false, IP_SRC_FLOW }, /* Flow-based everything. */
>> - { false, false, true, IP_SRC_EXACT }, /* remote_ip, local_ip,
>> in_key. */
>> - { false, false, true, IP_SRC_ANY }, /* remote_ip, in_key. */
>> - { true, false, true, IP_SRC_EXACT }, /* remote_ip, local_ip. */
>> - { true, false, true, IP_SRC_ANY }, /* remote_ip. */
>> - { true, true, true, IP_SRC_ANY }, /* Flow-based remote. */
>> - { true, true, true, IP_SRC_FLOW }, /* Flow-based everything. */
>> + { false, false, false, false, IP_SRC_EXACT }, /* remote_ip,
>> local_ip, in_key. */
>> + { false, false, false, false, IP_SRC_ANY }, /* remote_ip, in_key.
>> */
>> + { true, false, false, false, IP_SRC_EXACT }, /* remote_ip,
>> local_ip. */
>> + { true, false, false, false, IP_SRC_ANY }, /* remote_ip. */
>> + { true, true, false, false, IP_SRC_ANY }, /* Flow-based remote.
>> */
>> + { true, true, false, false, IP_SRC_FLOW }, /* Flow-based
>> everything. */
>> + { false, false, true, false, IP_SRC_EXACT }, /* remote_ip,
>> local_ip, in_key. */
>> + { false, false, true, false, IP_SRC_ANY }, /* remote_ip, in_key.
>> */
>> + { true, false, true, false, IP_SRC_EXACT }, /* remote_ip,
>> local_ip. */
>> + { true, false, true, false, IP_SRC_ANY }, /* remote_ip. */
>> + { true, true, true, false, IP_SRC_ANY }, /* Flow-based remote.
>> */
>> + { true, true, true, false, IP_SRC_FLOW }, /* Flow-based
>> everything. */
>> + { false, false, false, true, IP_SRC_EXACT }, /* remote_ip,
>> local_ip, in_key. */
>> + { false, false, false, true, IP_SRC_ANY }, /* remote_ip, in_key.
>> */
>> + { true, false, false, true, IP_SRC_EXACT }, /* remote_ip,
>> local_ip. */
>> + { true, false, false, true, IP_SRC_ANY }, /* remote_ip. */
>> + { true, true, false, true, IP_SRC_ANY }, /* Flow-based remote.
>> */
>> + { true, true, false, true, IP_SRC_FLOW }, /* Flow-based
>> everything. */
>> + { false, false, true, true, IP_SRC_EXACT }, /* remote_ip,
>> local_ip, in_key. */
>> + { false, false, true, true, IP_SRC_ANY }, /* remote_ip, in_key.
>> */
>> + { true, false, true, true, IP_SRC_EXACT }, /* remote_ip,
>> local_ip. */
>> + { true, false, true, true, IP_SRC_ANY }, /* remote_ip. */
>> + { true, true, true, true, IP_SRC_ANY }, /* Flow-based remote.
>> */
>> + { true, true, true, true, IP_SRC_FLOW }, /* Flow-based
>> everything. */
>> };
>>
>
> As you see, this is getting out of hand… Could you come up with descriptive
> comment for each case explaining why the permutation is needed? If not,
> then what does that tell you?
>
> I'm not claiming that the existing comments are very descriptive, btw.
as said in earlier email will document these and keep only those minimum
required, will remove rest of the cases.
>
>> const struct tnl_match_pattern *p;
>> @@ -462,6 +484,9 @@ tnl_find(const struct flow *flow) OVS_REQ_RDLOCK(rwlock)
>> match.in_nsp_flow = p->in_nsp_flow;
>> match.in_nsp = p->in_nsp_flow ? 0 : flow->tunnel.nsp;
>>
>> + match.in_nsi_flow = p->in_nsi_flow;
>> + match.in_nsi = p->in_nsi_flow ? 1 : flow->tunnel.nsi;
>> +
>> match.ip_dst_flow = p->ip_dst_flow;
>> match.ip_dst = p->ip_dst_flow ? 0 : flow->tunnel.ip_src;
>>
>
>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
