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".
> + 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.
>
…
> 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?
> + /* 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?
> 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.
> 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
