[ovs-dev] [netlink v4 40/52] datapath: Convert ODP_FLOW_* and ODP_EXECUTE to put dp_idx into message.

Jesse Gross jesse at nicira.com
Tue Jan 18 00:42:37 UTC 2011


On Mon, Jan 17, 2011 at 4:15 PM, Ben Pfaff <blp at nicira.com> wrote:
> On Mon, Jan 17, 2011 at 12:22:22PM -0500, Jesse Gross wrote:
>> On Wed, Jan 12, 2011 at 12:49 AM, Ben Pfaff <blp at nicira.com> wrote:
>> > diff --git a/datapath/datapath.c b/datapath/datapath.c
>> > index 4456ec8..ff2a6b8 100644
>> > --- a/datapath/datapath.c
>> > +++ b/datapath/datapath.c
>> > @@ -1619,6 +1662,30 @@ static long openvswitch_ioctl(struct file *f, unsigned int cmd,
>> > + ? ? ? case ODP_FLOW_FLUSH:
>> > + ? ? ? ? ? ? ? err = flush_flows(argp);
>> > + ? ? ? ? ? ? ? goto exit;
>>
>> I don't see the corresponding userspace changes to support this.  It
>> looks like argp will always be NULL.
>
> You're talking about ODP_FLOW_FLUSH?  There's no userspace interface
> change for this.  argp in this case is a dp_idx, not a pointer.  Yes,
> that's not like any of the other flow functions, but I didn't see a
> reason to change its interface yet.

It doesn't look like that is actually true:

static int
dpif_linux_flow_flush(struct dpif *dpif_)
{
    return do_ioctl(dpif_, ODP_FLOW_FLUSH, NULL);
}

Previously dp_idx was coming off the minor number, now it's coming
from argp which is always NULL, aka datapath 0.

>> > diff --git a/datapath/odp-compat.h b/datapath/odp-compat.h
>> > index bccfaf9..2f595db 100644
>> > --- a/datapath/odp-compat.h
>> > +++ b/datapath/odp-compat.h
>> > @@ -23,6 +23,7 @@
>> > ?#define ODP_FLOW_DEL32 ? ? ? ? _IOWR('O', 17, struct compat_odp_flow)
>> >
>> > ?struct compat_odp_flow {
>> > + ? ? ? int32_t dp_idx;
>> > ? ? ? ?struct odp_flow_stats stats;
>> > ? ? ? ?compat_uptr_t key;
>> > ? ? ? ?u32 key_len;
>> > @@ -36,11 +37,14 @@ struct compat_odp_flow_put {
>> > ?};
>> >
>> > ?struct compat_odp_flow_dump {
>> > + ? ? ? int32_t dp_idx;
>> > ? ? ? ?compat_uptr_t flow;
>> > ? ? ? ?uint32_t state[2];
>> > ?};
>> >
>> > ?struct compat_odp_execute {
>> > + ? ? ? int32_t dp_idx;
>> > +
>> > ? ? ? ?compat_uptr_t actions;
>> > ? ? ? ?u32 actions_len;
>>
>> It doesn't look like most of the compat code was updated to handle
>> these changes.
>
> Crap.
>
> I've added that now and I'm appending the new version of the patch.
> Compile-tested only, for now.

The new parts look good.

>
>> > diff --git a/include/openvswitch/datapath-protocol.h b/include/openvswitch/datapath-protocol.h
>> > index f852345..fbd32d1 100644
>> > --- a/include/openvswitch/datapath-protocol.h
>> > +++ b/include/openvswitch/datapath-protocol.h
>> > @@ -272,6 +272,7 @@ struct odp_key_arp {
>> > ?};
>> >
>> > ?struct odp_flow {
>> > + ? ?int32_t dp_idx;
>>
>> Why is this signed?
>
> Same reason as before in patch 27, equally valid or invalid.  I'll make
> the same change here when I resolve that one.

OK.




More information about the dev mailing list