| Message ID | 1500013851-89181-1-git-send-email-jpettit@ovn.org |
|---|---|
| State | Rejected |
| Headers | show |
On Thu, Jul 13, 2017 at 11:30:49PM -0700, Justin Pettit wrote: > Signed-off-by: Justin Pettit <jpettit@ovn.org> > --- > 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.
> On Jul 27, 2017, at 1:54 PM, Ben Pfaff <blp@ovn.org> wrote: > > On Thu, Jul 13, 2017 at 11:30:49PM -0700, Justin Pettit wrote: >> Signed-off-by: Justin Pettit <jpettit@ovn.org> >> --- >> 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
On Thu, Jul 27, 2017 at 02:05:22PM -0700, Justin Pettit wrote: > > > On Jul 27, 2017, at 1:54 PM, Ben Pfaff <blp@ovn.org> wrote: > > > > On Thu, Jul 13, 2017 at 11:30:49PM -0700, Justin Pettit wrote: > >> Signed-off-by: Justin Pettit <jpettit@ovn.org> > >> --- > >> 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.
> On Jul 27, 2017, at 4:17 PM, Ben Pfaff <blp@ovn.org> wrote: > > On Thu, Jul 27, 2017 at 02:05:22PM -0700, Justin Pettit wrote: >> >>> On Jul 27, 2017, at 1:54 PM, Ben Pfaff <blp@ovn.org> wrote: >>> >>> On Thu, Jul 13, 2017 at 11:30:49PM -0700, Justin Pettit wrote: >>>> Signed-off-by: Justin Pettit <jpettit@ovn.org> >>>> --- >>>> 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
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);
Signed-off-by: Justin Pettit <jpettit@ovn.org> --- ofproto/ofproto-dpif-rid.c | 1 + 1 file changed, 1 insertion(+)