[ovs-dev] [RFC PATCH 2/2] meta-flow: Add MFF_IN_PORT_OXM, a 32-bit in_port.

Ben Pfaff blp at nicira.com
Fri May 17 05:13:04 UTC 2013


On Fri, May 10, 2013 at 10:54:36PM +0300, Jarno Rajahalme wrote:
> This helps get rid of one special case in nx_pull_raw() and allows
> loading of 32-bit values to OXM_OF_IN_PORT in NXAST_LEARN actions.
> Previously the 16-bit limit acted the same on both NXM_OF_IN_PORT and
> OXM_OF_IN_PORT, even though OF1.1+ controllers would expect OXM_OF_IN_PORT
> to be 32 bits wide.

Thanks, this seems very reasonable.  I didn't know we had a bug in this
area.  Thanks for finding and fixing it and adding a test.  I think that
the patch can be improved, though, so some more comments follow.

I really like getting rid of the special case in nx_pull_raw().

> Signed-off-by: Jarno Rajahalme <jarno.rajahalme at nsn.com>
> ---
> I considered simply making the MFF_IN_PORT 32-bit instead, but that could
> have resulted in surprises for any existing use expecting it to be 16 bits
> (such as register load actions). Maybe there is no such conflict, and indeed
> it would be better to just widen the MFF_IN_PORT and add a special case for
> NXM_OF_IN_PORT into nx_pull_raw().

I think it's a bad idea to change the width of MFF_IN_PORT for exactly
the reason you mention.  I don't know whether there is much (any?) code
that actually does a move or load of NXM_OF_IN_PORT, but I also don't
know of a good reason to break that code if there is.

This check is pretty close to the implementation of
ofputil_port_from_ofp11().  The only real difference is a log message.
I think it would be better to try to avoid duplicating code:

> +    case MFF_IN_PORT_OXM: {
> +        uint32_t port = ntohl(value->be32);
> +        return port < OFPP_MAX || port >= OFPP11_MAX;
> +    }
> +
>      case MFF_IP_DSCP:
>          return !(value->u8 & ~IP_DSCP_MASK);
>      case MFF_IP_DSCP_SHIFTED:

The new port_from_ofp11() function is also very similar to
ofputil_port_from_ofp11(), so again I would prefer to avoid duplicating
code:
> +/* Map a 32 port number to 16-bit range. */
> +static inline uint16_t
> +port_from_ofp11(ovs_be32 ofp11_port)
> +{
> +  uint32_t port = ntohl(ofp11_port);
> +  return port < OFPP_MAX ? port
> +    : port >= OFPP11_MAX ? port - OFPP11_OFFSET
> +    : OFPP_NONE;
> +}

This code looks randomly indented and there is a missing {} around the
if's conditional statement.  I have my doubts about the correctness of
the logic; perhaps it would be simpler to use ofputil_port_to_ofp11() on
a random 16-bit value?
> +    case MFF_IN_PORT_OXM:
> +        value->be32 &= htonl(0x0000ffff);
> +	if (ntohl(value->be32) > OFPP_MAX)
> +	  value->be32 |= htonl(0xffff0000);
> +        break;
> +
>      case MFF_IPV6_LABEL:
>          value->be32 &= ~htonl(IPV6_LABEL_MASK);
>          break;

s/32_bit/32-bit/ here?
> +    MFS_OFP_PORT_OXM,           /* An OpenFlow port number or name (32_bit). */
>      MFS_FRAG,                   /* no, yes, first, later, not_later */
>      MFS_TNL_FLAGS,              /* FLOW_TNL_F_* flags */
>  };

Thanks,

Ben.



More information about the dev mailing list