On 18 September 2014 11:13, Ben Pfaff <b...@nicira.com> wrote:
> On Mon, Sep 15, 2014 at 02:25:14PM +1200, Joe Stringer wrote:
> > One of the limiting factors on the number of flows that can be supported
> > in the datapath is the overhead of assembling flow dump messages in the
> > datapath. This patch adds a new alternative to dumping the key, mask and
> > actions from the datapath, which is a 128-bit unique identifier for the
> > flow. For each flow dump, the dpif user specifies whether to skip these
> > attributes, allowing the common case to only dump a pair of 128-bit ID
> and
> > flow stats. This increases the number of flows that a revalidator can
> > handle per second by up to 40% with the linux datapath.
> >
> > The unique identifiers ("UID") for a flow are generated by the dpif.
> > During upcall processing, the dpif will provide a UID for each flow
> > which is based on the original flow key. During flow dump, if the
> > datapath provides the UID, then the dpif will pass it up transparently.
> > Otherwise, the dpif will generate the UID. This simplifies backward
> > compatibility handling in ofproto-dpif-upcall.
> >
> > Signed-off-by: Joe Stringer <joestrin...@nicira.com>
>
> I don't understand what's going on with the datapath interface.
>
I'll explain it here, but clearly I need to update the dpif.h comments as
well.
The DPIF now passes up a hash of the flow key to handlers/revalidators when
performing upcalls/flow dumps. (On master, handlers don't need a hash and
revalidators will generate it when they dump a flow). Handlers store it
with the ukey and use it for cmap indexing.
Flow_put now receives this UID, and passes it on to the datapath to store
with the flow.
Flow_get now receives a UID, and passes this down to the datapath to fetch
the specific flow. Flow_get requires at least one of UID or flow key, and
will pass down both to the datapath. In the case of dpif-netdev, it will
lookup based on the UID and if that fails, the operation fails. In
dpif-linux, it will attempt to use the UID first, then attempt to use the
specified flow_key for lookup, then fail if both of these fail.
Flow_delete now receives a UID and a flag to reduce overheads. Similar to
flow_get, it requires at least one of UID or flow_key and will attempte to
delete based on these the same way that flow_get attempts lookup. If the
'terse' flag for delete is false, then the operation is expected to return
the key, uid and stats. If 'terse' is true, then only the UID and stats is
required to be returned.
flow_dump_create() receives a new flag to determine whether entire flows
should be dumped, or whether a minimal set of information can be dumped. If
the 'terse' is false, then the dpif_flow_dump_next() is expected to return
entire flows including key,mask,actions. If 'terse' is true, then the
datapath is only required to return the UID and the stats. [For the linux
datapath, this is more granular, but that's not exposed to dpif]. I think I
dropped the actions logic in dpif_linux_flow_dump_next() which I now
realise needs to be restored for this interface to always act correctly.
For the most part, the revalidator logic doesn't care whether UID is
supported or not, and will pass down all information it has to the dpif. It
will also pass down the right flag for flow_delete or flow_dump_create() to
allow the dpif to skip transmitting some fields to the datapath. All fields
are still passed to dpif to allow the dpif logging to be user-friendly.
I had a quite different split of logical changes originally, but perhaps it
would be more logical (and would help review) to split this patch into:
A) Generating the hash in dpif, including it in upcalls/dumps and using it
for hashing in ofproto-dpif-upcall.c
B) Changing the put/get/delete interfaces to allow UID-indexed operations
C) Changing the flow_dump_create() and flow_dump_next() behaviour to
tersely dump flows
> The comment on ovs_flow_attr refers to OVS_DP_F_INDEX_BY_UID but there
> is no other reference to this in the patch.
>
That hunk is in the next patch. I can shift it forward into this patch.
> The comment on OVS_FLOW_ATTR_UID says that it is 64 bits, but the commit
> message says 128:
> + OVS_FLOW_ATTR_UID, /* 64-bit Unique flow identifier. */
> Actually I think that the comment on that should be more like:
> OVS_FLOW_ATTR_UID, /* Sequence of OVS_UID_ATTR_* attributes.
> */
> but I'm not really sure.
>
> Similar typo on OVS_UID_ATTR_ID:
> + OVS_UID_ATTR_ID, /* u64 unique flow identifier. */
>
> If you explain this stuff then I'll continue to review the patch.
>
Mixup from squashing patches. I investigated using a 64-bit identifier, but
switched back to this as the code was more straightforward. I'll fix this
up.
Cheers,
Joe
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
