[ovs-dev] [netlink v4 27/52] datapath: Change userspace vport interface to use Netlink attributes.
Ben Pfaff
blp at nicira.com
Wed Jan 19 00:25:09 UTC 2011
On Mon, Jan 17, 2011 at 03:21:45PM -0800, Jesse Gross wrote:
> Separately, I see the documentation for get/set_options both in
> vport.c and vport.h seems to be out of date. I also see that nothing
> actually calls vport_get_options(). Should we move that whole block
> of code relating to getting the options in copy_vport_to_user() to
> vport_get_options() and call that?
OK, done. Incremental patch at end of message.
> >> > diff --git a/include/openvswitch/datapath-protocol.h b/include/openvswitch/datapath-protocol.h
> >> > index b6ed979..ae82531 100644
> >> > --- a/include/openvswitch/datapath-protocol.h
> >> > +++ b/include/openvswitch/datapath-protocol.h
> >> > +struct odp_vport {
> >> > + ? ? ? int32_t dp_idx;
> >> > + ? ? ? int32_t port_no;
> >>
> >> Why are these signed?
> >
> > They are signed because we need to have values that mean "none
> > specified". ?At this point in the series, the value 0 is meaningful for
> > both dp_idx and port_no. ?Much later on (in one of the supplementary
> > series I sent out), dp_idx changes to an ifindex, so at that point I
> > start using 0 for a "none specified" value (and could potentially switch
> > to an unsigned type, except that the proper POSIX type for an ifindex is
> > "int" so actually I switch to that).
> >
> > We could switch to uint32_t and use UINT32_MAX as "none" if you prefer.
>
> I think where it makes sense we should use unsigned types, if only
> because signed types makes me wonder about the consequences of a
> negative index. Obviously, for things like ifindexes we should use
> the proper type.
OK, I changed these to uint32_t. I'll append the incremental.
> > This is related to the next issue, see below:
> >
> >> > + ? ? ? uint32_t type;
> >> > + ? ? ? uint32_t len;
> >> > + ? ? ? uint32_t total_len;
> >> > +};
> >> > +
> >> > +enum {
> >> > + ? ? ? ?ODPPT_UNSPEC,
> >> > + ? ? ? ?ODPPT_NAME, ? ? ? ? ? ?/* string name, up to IFNAMSIZ bytes long */
> >> > + ? ? ? ?ODPPT_STATS, ? ? ? ? ? /* struct rtnl_link_stats64 */
> >> > + ? ? ? ?ODPPT_ADDRESS, ? ? ? ? /* hardware address */
> >> > + ? ? ? ?ODPPT_MTU, ? ? ? ? ? ? /* 32-bit maximum transmission unit */
> >> > + ? ? ? ?ODPPT_OPTIONS, ? ? ? ? /* nested attributes, varies by vport type */
> >> > + ? ? ? ?ODPPT_IFINDEX, ? ? ? ? /* 32-bit ifindex of backing netdev */
> >> > + ? ? ? ?ODPPT_IFLINK, ? ? ? ? ?/* 32-bit ifindex on which packets are sent */
> >> > + ? ? ? ?__ODPPT_MAX
> >> > ?};
> >>
> >> The rationale for putting fields in the header vs attributes is not
> >> very clear to me (with the obvious exception of the length fields).
> >> It's perhaps not such a big deal here because we do need the fixed
> >> header at the beginning to hold the lengths. ?However, in the later
> >> patches where we actually use Netlink, this seems to lead unnecessary
> >> complexity. ?Having these fields in the header means that different
> >> operations have different headers, which means that we have different
> >> families. ?The end result is that we have a proliferation of families
> >> and structs. ?I would expect it to have one family with maybe a fixed
> >> header consisting of just the DP index.
> >
> > The difference between approaches is a matter of philosophy. ?I had
> > viewed each of the four families (datapath, vport, flow, upcall) as a
> > separate entity and thus given each of them a separate header and
> > family. ?But I can see that using a single family with a shared header
> > or perhaps no header at all would simplify other matters, as you say.
> >
> > There is no technical reason we cannot do it the way you suggest. ?It
> > had not occurred to me before to do it that way. ?If you prefer this
> > approach, then I will rework things to work that way instead.
>
> I guess I would prefer to make things a single family - I have a hard
> time logically justifying having so many different families and it
> seems that other parts of the kernel generally only have one.
> Combining them would shrink the interface, which is a pretty big win
> to me.
OK, I'll work on this.
> >> > diff --git a/include/openvswitch/tunnel.h b/include/openvswitch/tunnel.h
> >> > index 44facfa..1e7d2b4 100644
> >> > --- a/include/openvswitch/tunnel.h
> >> > +++ b/include/openvswitch/tunnel.h
> >> > @@ -43,6 +43,15 @@
> >> > ?#include <linux/types.h>
> >> > ?#include "openvswitch/datapath-protocol.h"
> >> >
> >> > +/* ODPPT_OPTIONS attributes for tunnels. */
> >> > +enum {
> >> > + ? ? ? TNLAT_UNSPEC,
> >> > + ? ? ? TNLAT_CONFIG, ? ? ? ? ? /* struct tnl_port_config */
> >>
> >> I think this really needs to be broken apart into separate attributes.
> >> ?I already have plans to change this struct, so not it's not realistic
> >> to expect it to stay the constant.
> >
> > OK, that's fine. ?Here's a proposal for attributes for you to review
> > before I go further along in implementation. ?I've retained the flags as
> > a 32-bit bitmask member; another way to do this with Netlink is to use
> > attributes that do not carry any data as flags:
>
> The attributes look good, I don't think there is much benefit in
> breaking apart the flags into separate attributes.
OK, thanks, I'll work on doing this.
Here's the incremental patch, which also includes a couple of other bug
fixes in attach_vport():
diff --git a/datapath/datapath.c b/datapath/datapath.c
index 9ccc311..6d9365e 100644
--- a/datapath/datapath.c
+++ b/datapath/datapath.c
@@ -1320,15 +1320,7 @@ static int copy_vport_to_user(void __user *dst, struct vport *vport, uint32_t to
NLA_PUT_U32(skb, ODPPT_MTU, vport_get_mtu(vport));
- if (vport->ops->get_options) {
- nla = nla_nest_start(skb, ODPPT_OPTIONS);
- if (!nla)
- goto nla_put_failure;
- err = vport->ops->get_options(vport, skb);
- if (err)
- goto exit_unlock;
- nla_nest_end(skb, nla);
- }
+ err = vport_get_options(vport, skb);
ifindex = vport_get_ifindex(vport);
if (ifindex > 0)
@@ -1433,7 +1425,7 @@ static struct vport *lookup_vport(struct odp_vport *odp_vport,
return vport;
} else {
- if (odp_vport->port_no < 0 || odp_vport->port_no >= DP_MAX_PORTS)
+ if (odp_vport->port_no >= DP_MAX_PORTS)
return ERR_PTR(-EINVAL);
dp = get_dp_locked(odp_vport->dp_idx);
@@ -1469,7 +1461,7 @@ static int attach_vport(struct odp_vport __user *uodp_vport)
struct vport *vport;
struct sk_buff *skb;
struct datapath *dp;
- int port_no;
+ u32 port_no;
int err;
skb = copy_vport_from_user(uodp_vport, a);
@@ -1490,12 +1482,12 @@ static int attach_vport(struct odp_vport __user *uodp_vport)
goto exit_unlock_rtnl;
port_no = odp_vport->port_no;
- if (port_no > 0) {
+ if (port_no < DP_MAX_PORTS) {
vport = get_vport_protected(dp, odp_vport->port_no);
err = -EBUSY;
if (vport)
goto exit_unlock_dp;
- } else {
+ } else if (port_no == (uint32_t)-1) {
for (port_no = 1; ; port_no++) {
if (port_no >= DP_MAX_PORTS) {
err = -EFBIG;
@@ -1505,6 +1497,9 @@ static int attach_vport(struct odp_vport __user *uodp_vport)
if (!vport)
break;
}
+ } else {
+ err = -EINVAL;
+ goto exit_unlock_dp;
}
parms.name = nla_data(a[ODPPT_NAME]);
@@ -1526,7 +1521,7 @@ static int attach_vport(struct odp_vport __user *uodp_vport)
dp_detach_port(vport);
exit_unlock_dp:
- mutex_unlock(&vport->dp->mutex);
+ mutex_unlock(&dp->mutex);
exit_unlock_rtnl:
rtnl_unlock();
exit_kfree_skb:
diff --git a/datapath/vport.c b/datapath/vport.c
index 177b723..7419038 100644
--- a/datapath/vport.c
+++ b/datapath/vport.c
@@ -696,20 +696,34 @@ int vport_get_mtu(const struct vport *vport)
}
/**
- * vport_get_options - retrieve device configuration
+ * vport_get_options - retrieve device options
*
- * @vport: vport from which to retrieve the configuration.
- * @config: buffer to store config, which must be at least the length
- * of VPORT_CONFIG_SIZE.
+ * @vport: vport from which to retrieve the options.
+ * @skb: sk_buff where options should be appended.
*
- * Retrieves the configuration of the given device. Either RTNL lock or
- * rcu_read_lock must be held.
+ * Retrieves the configuration of the given device, appending an %ODPPT_OPTIONS
+ * attribute that in turn contains nested vport-specific attributes to @skb.
+ * Either RTNL lock or rcu_read_lock must be held.
+ *
+ * Returns 0 if successful, -EMSGSIZE if @skb has insufficient room, or another
+ * negative error code if a real error occurred.
*/
int vport_get_options(const struct vport *vport, struct sk_buff *skb)
{
- if (!vport->ops->get_options)
- return 0;
- return vport->ops->get_options(vport, skb);
+ struct nlattr *nla;
+
+ nla = nla_nest_start(skb, ODPPT_OPTIONS);
+ if (!nla)
+ return -EMSGSIZE;
+
+ if (vport->ops->get_options) {
+ int err = vport->ops->get_options(vport, skb);
+ if (err)
+ return err;
+ }
+
+ nla_nest_end(skb, nla);
+ return 0;
}
/**
diff --git a/datapath/vport.h b/datapath/vport.h
index e9a27c6..dea5975 100644
--- a/datapath/vport.h
+++ b/datapath/vport.h
@@ -152,9 +152,12 @@ struct vport_parms {
* @exit: Called at module unload.
* @create: Create a new vport configured as specified. On success returns
* a new vport allocated with vport_alloc(), otherwise an ERR_PTR() value.
- * @modify: Modify the configuration of an existing vport. May be null if
- * modification is not supported.
* @destroy: Detach and destroy a vport.
+ * @set_options: Modify the configuration of an existing vport. May be %NULL
+ * if modification is not supported.
+ * @get_options: Appends vport-specific attributes for the configuration of an
+ * existing vport to a &struct sk_buff. May be %NULL for a vport that does not
+ * have any configuration.
* @set_mtu: Set the device's MTU. May be null if not supported.
* @set_addr: Set the device's MAC address. May be null if not supported.
* @set_stats: Provides stats as an offset to be added to the device stats.
diff --git a/include/openvswitch/datapath-protocol.h b/include/openvswitch/datapath-protocol.h
index ae82531..36cad3d 100644
--- a/include/openvswitch/datapath-protocol.h
+++ b/include/openvswitch/datapath-protocol.h
@@ -193,8 +193,8 @@ enum odp_vport_type {
* up to a length of @len bytes including the &struct odp_vport header.
*/
struct odp_vport {
- int32_t dp_idx;
- int32_t port_no;
+ uint32_t dp_idx;
+ uint32_t port_no;
uint32_t type;
uint32_t len;
uint32_t total_len;
diff --git a/lib/dpif-linux.c b/lib/dpif-linux.c
index 39e123e..ede7d61 100644
--- a/lib/dpif-linux.c
+++ b/lib/dpif-linux.c
@@ -1001,6 +1001,8 @@ void
dpif_linux_vport_init(struct dpif_linux_vport *vport)
{
memset(vport, 0, sizeof *vport);
+ vport->dp_idx = UINT32_MAX;
+ vport->port_no = UINT32_MAX;
}
/* Executes 'request' in the kernel datapath. If the command fails, returns a
diff --git a/lib/dpif-linux.h b/lib/dpif-linux.h
index 552f432..7d9e69b 100644
--- a/lib/dpif-linux.h
+++ b/lib/dpif-linux.h
@@ -27,8 +27,8 @@ struct dpif_linux_vport {
int cmd;
/* odp_vport header. */
- int dp_idx;
- int port_no;
+ uint32_t dp_idx;
+ uint32_t port_no; /* UINT32_MAX if unknown. */
enum odp_vport_type type;
/* Attributes. */
More information about the dev
mailing list