Re: [ovs-dev] [PATCH] Use "error-checking" mutexes in place of other kinds wherever possible.
Oops yes you're right. It might be worth defining a separate function for that, but its your call. The patch is good as is. Ethan (iPhone) On Aug 17, 2013, at 13:08, Ben Pfaff wrote: > > On Aug 16, 2013 9:47 PM, "Ethan Jackson" wrote: > > Personally I'd prefer we change ovs_mutex_init() to not take a mutex > > type as an argument and simply use the error checking mutex always. I > > think it's going to be a long time before we actually need to > > configure this on a per mutex basis, and I don't think we know what > > abstraction we'll need at that point today. > > Don't we need the ability to initialize a recursive mutex? It didn't occur to > me to drop the parameter but that's the only current reason to keep it. > > Thanks, > > Ben. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [PATCH] tests: Fix build on FreeBSD
Avoid relying on a non-portable implementation detail for atomic_flag
tests. Per the standard, the only way to obtain the value of the flag
is via the return value from atomic_flag_test_and_set.
Signed-off-by: Ed Maste
---
tests/test-atomic.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/tests/test-atomic.c b/tests/test-atomic.c
index e9bd6bd..e6df1cd 100644
--- a/tests/test-atomic.c
+++ b/tests/test-atomic.c
@@ -66,9 +66,9 @@ test_atomic_flag(void)
{
atomic_flag flag = ATOMIC_FLAG_INIT;
ovs_assert(atomic_flag_test_and_set(&flag) == false);
-ovs_assert(flag.b == true);
+ovs_assert(atomic_flag_test_and_set(&flag) == true);
atomic_flag_clear(&flag);
-ovs_assert(flag.b == false);
+ovs_assert(atomic_flag_test_and_set(&flag) == false);
}
int
--
1.8.3.3
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] tests: Fix build on FreeBSD
On Sat, Aug 17, 2013 at 04:40:11PM -0400, Ed Maste wrote: > Avoid relying on a non-portable implementation detail for atomic_flag > tests. Per the standard, the only way to obtain the value of the flag > is via the return value from atomic_flag_test_and_set. > > Signed-off-by: Ed Maste Thanks, applied. Sorry about letting that sneak in, it's my fault. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] OF1.1+ Groups
On Sun, Aug 04, 2013 at 07:45:38PM -0700, Ben Pfaff wrote: > On Mon, Aug 05, 2013 at 11:38:57AM +0900, Simon Horman wrote: > > Hi, > > > > I would like to announce my intention to work on OF1.1+ Groups support for > > Open vSwtich with a particular focus on supporting the fast failover group > > type. > > > > I do not wish to tread on any toes so if anyone is already working on this > > please let me know. > > > > Assuming that is not the case my high-level plan is as follows: > > > > 1. Test and as necessary fix > >"[groups RFC 2/2] Implement OpenFLow 1.1+ "groups" protocol."[1] > >by Neil Zhu. > > I'm working on this myself and made some progress. I'll try to post a > new version this week. Hi Ben, I am wondering if you managed to make any progress on this. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [thread 09/15] ofproto-dpif: Lock the expirable list.
On Thu, Aug 08, 2013 at 02:37:48PM -0700, Ben Pfaff wrote: > On Thu, Aug 08, 2013 at 02:32:08PM -0700, Ethan Jackson wrote: > > > Simon presented numbers that showed it to be a valuable optimization > > > in some cases, otherwise I'd just say get rid of it. > > > > If that's the only reason we have it, I vote we ditch it. It's a new > > world with multithreading, I'd like to have a clean slate in the > > direction of thread safety and add optimizations back if they help in > > this new paradigm > > That's the only reason we have it. If you want to ditch it be my > guest. I accept that multi-threaded ovs-vswtichd is a whole new world and that dropping optimisations that previously made sense is logical. And I guess that the best thing is for the situation that lead to this optimisation to be re-profiled once the multi-threading work is more complete. For reference, my recollection is that this optimisation came about because it was observed that inserting 100k non-expirable flows would result in ovs-vswtichd consuming about 100% (of one) CPU running through the list of flows to see if any of them were ready to be expired although none of them ever would be. With the optimisation in place CPU utilisation was reduced to roughly 0% as the list that was traversed became empty and the system was otherwise idle. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] Introduce odp_flow_key_to_mask() API
On Wed, Aug 14, 2013 at 03:36:25PM -0700, Andy Zhou wrote:
> You may want to combine the two patches into one.
>
> It is not good to have is_zero mask check scattered over. This will hard to
> maintain.
>
>
> On Tue, Aug 13, 2013 at 2:50 PM, wrote:
>
> > From: gyang
> >
> > With megaflow support, there is API to convert mask to nlattr key based
> > format. This change introduces API to do the reverse conversion. We
> > leverage the existing odp_flow_key_to_flow() API to resue the code.
> >
> > Signed-off-by: Guolin Yang
> > ---
> > lib/odp-util.c | 257
> > +++-
> > lib/odp-util.h |2 +
> > 2 files changed, 184 insertions(+), 75 deletions(-)
> >
> > diff --git a/lib/odp-util.c b/lib/odp-util.c
> > index 78d5a1b..3a1b55c 100644
> > --- a/lib/odp-util.c
> > +++ b/lib/odp-util.c
> > @@ -2680,20 +2680,30 @@ check_expectations(uint64_t present_attrs, int
> > out_of_range_attr,
> > static bool
> > parse_ethertype(const struct nlattr *attrs[OVS_KEY_ATTR_MAX + 1],
> > uint64_t present_attrs, uint64_t *expected_attrs,
> > -struct flow *flow)
> > +struct flow *flow, struct flow *src_flow)
> > {
> > static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
> > +bool is_mask = flow != src_flow;
> >
> > if (present_attrs & (UINT64_C(1) << OVS_KEY_ATTR_ETHERTYPE)) {
> > flow->dl_type = nl_attr_get_be16(attrs[OVS_KEY_ATTR_ETHERTYPE]);
> > -if (ntohs(flow->dl_type) < 1536) {
> > +if (!is_mask && ntohs(flow->dl_type) < 1536) {
> >
> Is possible not to hard code 1536?
ETH_TYPE_MIN in packet.h may be useful here.
Or possibly ETH_P_802_3_MIN in linux/if_ether.h
> > VLOG_ERR_RL(&rl, "invalid Ethertype %"PRIu16" in flow key",
> > ntohs(flow->dl_type));
> > return false;
> > }
> > *expected_attrs |= UINT64_C(1) << OVS_KEY_ATTR_ETHERTYPE;
> > } else {
> > -flow->dl_type = htons(FLOW_DL_TYPE_NONE);
> > +if (!is_mask) {
> > +flow->dl_type = htons(FLOW_DL_TYPE_NONE);
> > +} else if (ntohs(src_flow->dl_type) < 1536) {
> >
> if mask is exact match, we should accept dl_type to be 802.2.
>
> > +/*
> > + * see comments odp_flow_key_from_flow__
> > + */
> > +VLOG_ERR_RL(&rl, "Alway expect mask for non ethernet II "
> > +"frame");
> > +return false;
> > +}
> > }
> > return true;
> > }
> > @@ -2702,38 +2712,63 @@ static enum odp_key_fitness
> > parse_l2_5_onward(const struct nlattr *attrs[OVS_KEY_ATTR_MAX + 1],
> >uint64_t present_attrs, int out_of_range_attr,
> >uint64_t expected_attrs, struct flow *flow,
> > - const struct nlattr *key, size_t key_len)
> > + const struct nlattr *key, size_t key_len,
> > + struct flow *src_flow)
> > {
> > static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
> > -
> > -if (eth_type_mpls(flow->dl_type)) {
> > -expected_attrs |= (UINT64_C(1) << OVS_KEY_ATTR_MPLS);
> > -
> > -if (!(present_attrs & (UINT64_C(1) << OVS_KEY_ATTR_MPLS))) {
> > -return ODP_FIT_TOO_LITTLE;
> > +bool is_mask = src_flow != flow;
> > +
> > +if (is_mask && flow->dl_type != 0x) {
> > +goto done;
> > +}
> > +if (eth_type_mpls(src_flow->dl_type)) {
> > + if (!is_mask) {
> > + expected_attrs |= (UINT64_C(1) << OVS_KEY_ATTR_MPLS);
> > +
> > + if (!(present_attrs & (UINT64_C(1) << OVS_KEY_ATTR_MPLS))) {
> > + return ODP_FIT_TOO_LITTLE;
> >
> Can we combine this check to the end, and even for mask? By the time all
> netlink attributes are processed
> expected_attrs == present_attrs should always to be true, right?
>
> > + }
> > + flow->mpls_lse = nl_attr_get_be32(attrs[OVS_KEY_ATTR_MPLS]);
> > + flow->mpls_depth++;
> > +} else if (present_attrs & (UINT64_C(1) << OVS_KEY_ATTR_MPLS)) {
> > +expected_attrs |= (UINT64_C(1) << OVS_KEY_ATTR_MPLS);
> > +flow->mpls_lse = nl_attr_get_be32(attrs[OVS_KEY_ATTR_MPLS]);
> > +/*
> > + * do we need to set the following? XXX
> > + */
> > +flow->mpls_depth = 0x;
> > +}
> > +} else if (src_flow->dl_type == htons(ETH_TYPE_IP)) {
> > +if (!is_mask) {
> > +expected_attrs |= UINT64_C(1) << OVS_KEY_ATTR_IPV4;
> > }
> > -flow->mpls_lse = nl_attr_get_be32(attrs[OVS_KEY_ATTR_MPLS]);
> > -flow->mpls_depth++;
> > -} else if (flow->dl_type == htons(ETH_TYPE_IP)) {
> > -expected_attrs |= UINT64_C(1) << OVS_KEY_ATTR_IPV4;
> > if (present_attrs & (UINT64_C(1) << OVS_KEY_ATTR_IPV4)) {
> > const struct ovs_key_ipv4 *ipv4_key;
> >
> > + if (is_
