[ovs-dev] [encap v2 2/3] datapath: Describe policy for extending flow key, implement needed changes.

Ben Pfaff blp at nicira.com
Mon Nov 14 23:20:14 UTC 2011


On Mon, Nov 14, 2011 at 02:37:37PM -0800, Jesse Gross wrote:
> On Sat, Nov 12, 2011 at 1:01 PM, Ben Pfaff <blp at nicira.com> wrote:
> > When the datapath was converted to use Netlink attributes for describing
> > flow keys, I had a vague idea of how it could be smoothly extensible, but
> > I didn't actually implement extensibility or carefully think it through.
> > This commit adds a document that describes how flow keys can be extended
> > in a compatible fashion and adapts the existing interface to match what
> > it says.
> >
> > This commit doesn't actually implement extensibility. ??I already have a
> > separate patch series out for that. ??This patch series borrows from that
> > one heavily, but the extensibility series will need to be reworked
> > somewhat once this one is in.
> >
> > This commit is only lightly tested because I don't have a good test setup
> > for VLANs.
> >
> > Signed-off-by: Ben Pfaff <blp at nicira.com>
> 
> I got some sparse errors with this:
> /home/jesse/openvswitch/datapath/linux/flow.c:1091:29: warning: symbol
> 'err' shadows an earlier one
> /home/jesse/openvswitch/datapath/linux/flow.c:1010:13: originally declared here
> /home/jesse/openvswitch/datapath/linux/flow.c:1117:29: warning: symbol
> 'err' shadows an earlier one
> /home/jesse/openvswitch/datapath/linux/flow.c:1010:13: originally declared here

Odd, I still don't see those.  Do you use any particular sparse flags?

Fixed, anyway, by s/int err = /err =/ on those two lines.

> > diff --git a/datapath/actions.c b/datapath/actions.c
> > index ac7187b..f3d7de9 100644
> > --- a/datapath/actions.c
> > +++ b/datapath/actions.c
> > @@ -368,14 +368,13 @@ static int do_execute_actions(struct datapath *dp, struct sk_buff *skb,
> [...]
> > - ?? ?? ?? ?? ?? ?? ?? case OVS_ACTION_ATTR_POP:
> > + ?? ?? ?? ?? ?? ?? ?? case OVS_ACTION_ATTR_POP_VLAN:
> > ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ??/* Only supported pop action is on vlan tag. */
> > ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ??err = pop_vlan(skb);
> 
> Since there's no longer a generic pop action I think we can remove this comment.

Good point, I've deleted that line.

> > diff --git a/datapath/datapath.c b/datapath/datapath.c
> > index 49d93aa..a21b0fa 100644
> > --- a/datapath/datapath.c
> > +++ b/datapath/datapath.c
> > + ?? ?? ?? ?? ?? ?? ?? case OVS_ACTION_ATTR_PUSH_VLAN:
> > + ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? if (nla_get_be16(a) & htons(VLAN_TAG_PRESENT))
> > ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ??return -EINVAL;
> > ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ??break;
> 
> Don't we still want to pass down the TPID for push vlan actions?  I
> think the previous model for push vlan where we had the TPID and TCI
> together was actually ideal, it was just the generic part that was
> problematic.

OK, I was unsure.  I'll make that change and send out a new version.

> > diff --git a/datapath/flow.c b/datapath/flow.c
> > index 6caca2a..4d16caa 100644
> > --- a/datapath/flow.c
> > +++ b/datapath/flow.c
> > @@ -1201,15 +1216,15 @@ int flow_to_nlattrs(const struct sw_flow_key *swkey, struct sk_buff *skb)
> > ?? ?? ?? ??if (swkey->eth.tci != htons(0)) {
> > - ?? ?? ?? ?? ?? ?? ?? struct ovs_key_8021q q_key;
> > -
> > - ?? ?? ?? ?? ?? ?? ?? q_key.q_tpid = htons(ETH_P_8021Q);
> > - ?? ?? ?? ?? ?? ?? ?? q_key.q_tci = swkey->eth.tci & ~htons(VLAN_TAG_PRESENT);
> > - ?? ?? ?? ?? ?? ?? ?? NLA_PUT(skb, OVS_KEY_ATTR_8021Q, sizeof(q_key), &q_key);
> > - ?? ?? ?? }
> > + ?? ?? ?? ?? ?? ?? ?? NLA_PUT_BE16(skb, OVS_KEY_ATTR_ETHERTYPE, htons(ETH_P_8021Q));
> > + ?? ?? ?? ?? ?? ?? ?? NLA_PUT_BE16(skb, OVS_KEY_ATTR_VLAN,
> > + ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ??swkey->eth.tci & ~htons(VLAN_TAG_PRESENT));
> > + ?? ?? ?? ?? ?? ?? ?? encap = nla_nest_start(skb, OVS_KEY_ATTR_ENCAP);
> > + ?? ?? ?? } else
> > + ?? ?? ?? ?? ?? ?? ?? encap = NULL;
> 
> Another one-armed if statement here.

Fixed, thanks.

> > diff --git a/include/linux/openvswitch.h b/include/linux/openvswitch.h
> > index 966ef7a..3ac6673 100644
> > --- a/include/linux/openvswitch.h
> > +++ b/include/linux/openvswitch.h
> > @@ -267,7 +267,7 @@ enum ovs_key_attr {
> > ?? ?? ?? ??OVS_KEY_ATTR_PRIORITY, ??/* u32 skb->priority */
> > ?? ?? ?? ??OVS_KEY_ATTR_IN_PORT, ?? /* u32 OVS dp port number */
> > ?? ?? ?? ??OVS_KEY_ATTR_ETHERNET, ??/* struct ovs_key_ethernet */
> > - ?? ?? ?? OVS_KEY_ATTR_8021Q, ?? ?? /* struct ovs_key_8021q */
> > + ?? ?? ?? OVS_KEY_ATTR_VLAN, ?? ?? ??/* be16 VLAN TCI */
> > ?? ?? ?? ??OVS_KEY_ATTR_ETHERTYPE, /* be16 Ethernet type */
> > ?? ?? ?? ??OVS_KEY_ATTR_IPV4, ?? ?? ??/* struct ovs_key_ipv4 */
> > ?? ?? ?? ??OVS_KEY_ATTR_IPV6, ?? ?? ??/* struct ovs_key_ipv6 */
> > @@ -277,6 +277,7 @@ enum ovs_key_attr {
> > ?? ?? ?? ??OVS_KEY_ATTR_ICMPV6, ?? ??/* struct ovs_key_icmpv6 */
> > ?? ?? ?? ??OVS_KEY_ATTR_ARP, ?? ?? ?? /* struct ovs_key_arp */
> > ?? ?? ?? ??OVS_KEY_ATTR_ND, ?? ?? ?? ??/* struct ovs_key_nd */
> > + ?? ?? ?? OVS_KEY_ATTR_ENCAP, ?? ?? /* Nested set of encapsulated attributes. */
> 
> Should we put this closer to the beginning of the list rather than
> just mixed in the middle?

I moved it to the top, right after UNSPEC.  Do you want it somewhere
else?

> > diff --git a/lib/odp-util.c b/lib/odp-util.c
> > index c70ab11..0ca616b 100644
> > --- a/lib/odp-util.c
> > +++ b/lib/odp-util.c
> > +parse_flow_nlattrs(const struct nlattr *key, size_t key_len,
> > + ?? ?? ?? ?? ?? ?? ?? ?? ?? const struct nlattr *attrs[], uint64_t *present_attrsp)
> > ??{
> > ?? ?? static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
> > - ?? ??const struct nlattr *attrs[OVS_KEY_ATTR_MAX + 1];
> > ?? ?? const struct nlattr *nla;
> > - ?? ??uint64_t expected_attrs;
> > ?? ?? uint64_t present_attrs;
> > - ?? ??uint64_t missing_attrs;
> > - ?? ??uint64_t extra_attrs;
> > ?? ?? size_t left;
> >
> > - ?? ??memset(flow, 0, sizeof *flow);
> > -
> > - ?? ??memset(attrs, 0, sizeof attrs);
> > + ?? ??memset(attrs, 0, (OVS_KEY_ATTR_MAX + 1) * sizeof *attrs);
> 
> Is there a reason why userspace and kernel do duplicate checking
> differently?  The kernel does it based on present_attrs and userspace
> does it based on the attribute stored in the array.

I didn't want the overhead of memset'ing all 64*4 == 256 or 64*8 ==
512 bytes of the temporary array in the kernel, so I used the bitmap
exclusively there to keep track of whether an attribute had been seen.
But I'll change it to whichever way you prefer.



More information about the dev mailing list