Re: [ovs-dev] [PATCH 1/3] ofproto-dpif-rid: Include action length as part of hash.

[Justin Pettit]

> On Jul 27, 2017, at 4:17 PM, Ben Pfaff  wrote:
> 
> On Thu, Jul 27, 2017 at 02:05:22PM -0700, Justin Pettit wrote:
>> 
>>> On Jul 27, 2017, at 1:54 PM, Ben Pfaff  wrote:
>>> 
>>> On Thu, Jul 13, 2017 at 11:30:49PM -0700, Justin Pettit wrote:
 Signed-off-by: Justin Pettit 
 ---
 ofproto/ofproto-dpif-rid.c | 1 +
 1 file changed, 1 insertion(+)
 
 diff --git a/ofproto/ofproto-dpif-rid.c b/ofproto/ofproto-dpif-rid.c
 index d546b150b938..26c2357007b2 100644
 --- a/ofproto/ofproto-dpif-rid.c
 +++ b/ofproto/ofproto-dpif-rid.c
 @@ -146,6 +146,7 @@ frozen_state_hash(const struct frozen_state *state)
hash = hash_bytes64(ALIGNED_CAST(const uint64_t *, 
 state->action_set),
state->action_set_len, hash);
}
 +hash = hash_int(state->ofpacts_len, hash);
if (state->ofpacts_len) {
hash = hash_bytes64(ALIGNED_CAST(const uint64_t *, state->ofpacts),
state->ofpacts_len, hash);
>>> 
>>> Can you explain the benefit of this change?  hash_bytes64() already uses
>>> the number of bytes hashed as one of the inputs to the hash.
>> 
>> hash_bytes64() is only called if the action length is non-zero.
> 
> Yes, but even so, the hash should still distinguish states with no
> actions from states with actions.

Yes, I agree.

>> However, I was on the fence about making this change, since it wasn't
>> clear if it would be that valuable.  The main reason was just to make
>> it consistent with how "action_set" is handled right above it.
>> 
>> I'm happy to drop the patch if you prefer.
> 
> I'd be more inclined to remove the hash of action_set_len, because it is
> unnecessary for the same reason that hash of ofpacts_len is unnecessary.

I'll go ahead and do that.  I'll send out a v2, since I need to respin the next 
patch in the series due to a build issue on some versions of gcc.

--Justin


___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH 1/3] ofproto-dpif-rid: Include action length as part of hash.

[Ben Pfaff]

On Thu, Jul 27, 2017 at 02:05:22PM -0700, Justin Pettit wrote:
> 
> > On Jul 27, 2017, at 1:54 PM, Ben Pfaff  wrote:
> > 
> > On Thu, Jul 13, 2017 at 11:30:49PM -0700, Justin Pettit wrote:
> >> Signed-off-by: Justin Pettit 
> >> ---
> >> ofproto/ofproto-dpif-rid.c | 1 +
> >> 1 file changed, 1 insertion(+)
> >> 
> >> diff --git a/ofproto/ofproto-dpif-rid.c b/ofproto/ofproto-dpif-rid.c
> >> index d546b150b938..26c2357007b2 100644
> >> --- a/ofproto/ofproto-dpif-rid.c
> >> +++ b/ofproto/ofproto-dpif-rid.c
> >> @@ -146,6 +146,7 @@ frozen_state_hash(const struct frozen_state *state)
> >> hash = hash_bytes64(ALIGNED_CAST(const uint64_t *, 
> >> state->action_set),
> >> state->action_set_len, hash);
> >> }
> >> +hash = hash_int(state->ofpacts_len, hash);
> >> if (state->ofpacts_len) {
> >> hash = hash_bytes64(ALIGNED_CAST(const uint64_t *, state->ofpacts),
> >> state->ofpacts_len, hash);
> > 
> > Can you explain the benefit of this change?  hash_bytes64() already uses
> > the number of bytes hashed as one of the inputs to the hash.
> 
> hash_bytes64() is only called if the action length is non-zero.

Yes, but even so, the hash should still distinguish states with no
actions from states with actions.

> However, I was on the fence about making this change, since it wasn't
> clear if it would be that valuable.  The main reason was just to make
> it consistent with how "action_set" is handled right above it.
>
> I'm happy to drop the patch if you prefer.

I'd be more inclined to remove the hash of action_set_len, because it is
unnecessary for the same reason that hash of ofpacts_len is unnecessary.

Thanks,

Ben.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH 1/3] ofproto-dpif-rid: Include action length as part of hash.

[Justin Pettit]

> On Jul 27, 2017, at 1:54 PM, Ben Pfaff  wrote:
> 
> On Thu, Jul 13, 2017 at 11:30:49PM -0700, Justin Pettit wrote:
>> Signed-off-by: Justin Pettit 
>> ---
>> ofproto/ofproto-dpif-rid.c | 1 +
>> 1 file changed, 1 insertion(+)
>> 
>> diff --git a/ofproto/ofproto-dpif-rid.c b/ofproto/ofproto-dpif-rid.c
>> index d546b150b938..26c2357007b2 100644
>> --- a/ofproto/ofproto-dpif-rid.c
>> +++ b/ofproto/ofproto-dpif-rid.c
>> @@ -146,6 +146,7 @@ frozen_state_hash(const struct frozen_state *state)
>> hash = hash_bytes64(ALIGNED_CAST(const uint64_t *, 
>> state->action_set),
>> state->action_set_len, hash);
>> }
>> +hash = hash_int(state->ofpacts_len, hash);
>> if (state->ofpacts_len) {
>> hash = hash_bytes64(ALIGNED_CAST(const uint64_t *, state->ofpacts),
>> state->ofpacts_len, hash);
> 
> Can you explain the benefit of this change?  hash_bytes64() already uses
> the number of bytes hashed as one of the inputs to the hash.

hash_bytes64() is only called if the action length is non-zero.  However, I was 
on the fence about making this change, since it wasn't clear if it would be 
that valuable.  The main reason was just to make it consistent with how 
"action_set" is handled right above it. 

I'm happy to drop the patch if you prefer.

--Justin


___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH 1/3] ofproto-dpif-rid: Include action length as part of hash.

[Ben Pfaff]

On Thu, Jul 13, 2017 at 11:30:49PM -0700, Justin Pettit wrote:
> Signed-off-by: Justin Pettit 
> ---
>  ofproto/ofproto-dpif-rid.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/ofproto/ofproto-dpif-rid.c b/ofproto/ofproto-dpif-rid.c
> index d546b150b938..26c2357007b2 100644
> --- a/ofproto/ofproto-dpif-rid.c
> +++ b/ofproto/ofproto-dpif-rid.c
> @@ -146,6 +146,7 @@ frozen_state_hash(const struct frozen_state *state)
>  hash = hash_bytes64(ALIGNED_CAST(const uint64_t *, 
> state->action_set),
>  state->action_set_len, hash);
>  }
> +hash = hash_int(state->ofpacts_len, hash);
>  if (state->ofpacts_len) {
>  hash = hash_bytes64(ALIGNED_CAST(const uint64_t *, state->ofpacts),
>  state->ofpacts_len, hash);

Can you explain the benefit of this change?  hash_bytes64() already uses
the number of bytes hashed as one of the inputs to the hash.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH 1/3] ofproto-dpif-rid: Include action length as part of hash.

[Justin Pettit]

Signed-off-by: Justin Pettit 
---
 ofproto/ofproto-dpif-rid.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/ofproto/ofproto-dpif-rid.c b/ofproto/ofproto-dpif-rid.c
index d546b150b938..26c2357007b2 100644
--- a/ofproto/ofproto-dpif-rid.c
+++ b/ofproto/ofproto-dpif-rid.c
@@ -146,6 +146,7 @@ frozen_state_hash(const struct frozen_state *state)
 hash = hash_bytes64(ALIGNED_CAST(const uint64_t *, state->action_set),
 state->action_set_len, hash);
 }
+hash = hash_int(state->ofpacts_len, hash);
 if (state->ofpacts_len) {
 hash = hash_bytes64(ALIGNED_CAST(const uint64_t *, state->ofpacts),
 state->ofpacts_len, hash);
-- 
2.7.4

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev