On Mon, Nov 14, 2011 at 02:37:37PM -0800, Jesse Gross wrote:
> On Sat, Nov 12, 2011 at 1:01 PM, Ben Pfaff <b...@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 <b...@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.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
