On Fri, Nov 01, 2013 at 02:15:13AM -0700, Gurucharan Shetty wrote:
> Instead of an exact match flow table, we introduce a classifier.
> This enables mega-flows in userspace datapath.
>
> Signed-off-by: Gurucharan Shetty <gshe...@nicira.com>
Clang says:
../lib/dpif-netdev.c:638:5: error: calling function 'classifier_remove'
requires exclusive lock on 'dp->cls.rwlock' [-Werror,-Wthread-safety-analysis]
classifier_remove(&dp->cls, &netdev_flow->cr);
^
../lib/dpif-netdev.c:752:10: error: calling function 'classifier_lookup'
requires shared lock on 'dp->cls.rwlock' [-Werror,-Wthread-safety-analysis]
cr = classifier_lookup(&dp->cls, flow, NULL);
^
../lib/dpif-netdev.c:882:5: error: calling function 'classifier_insert'
requires exclusive lock on 'dp->cls.rwlock' [-Werror,-Wthread-safety-analysis]
classifier_insert(&dp->cls, &netdev_flow->cr);
^
../lib/dpif-netdev.c:886:9: error: calling function 'classifier_remove'
requires exclusive lock on 'dp->cls.rwlock' [-Werror,-Wthread-safety-analysis]
classifier_remove(&dp->cls, &netdev_flow->cr);
^
All the flows in the classifier will have the same priority, right? I
think so. The "default" here, to my eyes, implies that the default
can be overridden:
> +/* By default, choose a priority in the middle. */
> +#define NETDEV_DEFAULT_PRIORITY 0x8000
I know why struct dp_netdev_flow needs 'flow' in addition to a
cls_rule: because a cls_rule always zeros out the bits in the flow,
and dpif-netdev needs to be able to report the original values of
those bits. But why does struct dp_netdev_flow need another copy of
the mask?
I don't really understand dp_netdev_lookup_flow(). It is being called
in two different and conflicting contexts. dp_netdev_port_input()
uses it to determine how to handle an incoming packet, which correctly
would just call classifier_lookup(). The other functions that call
dp_netdev_lookup_flow() should just call
classifier_find_rule_exactly(). (Or am I missing something
important?)
Also in dpif_netdev_flow_mask_from_nlattrs(), should we now only check
that the in_port is valid, if the in_port is not wildcarded?
The wrapper in the conditional here looks odd to me, because the
indentation does not reflect the logical structure of the test:
if (odp_flow_key_to_flow(key, key_len, flow) || (mask_key &&
odp_flow_key_to_mask(mask_key, mask_key_len, mask, flow))) {
I would write it as, say:
if (odp_flow_key_to_flow(key, key_len, flow)
|| (mask_key
&& odp_flow_key_to_mask(mask_key, mask_key_len, mask, flow))) {
although || and && at the ends of lines instead of at the beginning is
fine too if you prefer.
In dp_netdev_flow_add(), copying the mask into a flow_wildcards
structure seems wasteful. I think that the caller can easily provide
the mask in that form, without copying.
Thanks,
Ben.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
