[ovs-dev] [PATCH ovn v3 3/5] meta-flow: Add new functions for subvalues.

Andy Zhou azhou at nicira.com
Wed Apr 15 19:36:40 UTC 2015


Thanks for explaining.  Given av == av & am is a prerequisite, this
function looks fine.

For the patch:
Acked-by: Andy Zhou <azhou at nicira.com>

On Wed, Apr 15, 2015 at 11:35 AM, Ben Pfaff <blp at nicira.com> wrote:
> This combination of av with am is invalid because av has a 1-bit (its
> least-significant bit) where am has a 0-bit.  Suppose that we pretend
> that am is instead 1001, which would make av/am valid.  Then av/am
> requires the least-significant bit to be 1 and bv/bm requires it to be
> 0.  This is the "impossible" case described in the function-level
> comment, which makes the function return false because:
>
>     (av ^ bv) & (am & bm)
>  == (1001 ^ 1000) & (1001 & 1001)
>  == 0001 & 1001
>  == 1
>
> which is nonzero, hence the if statement returns false.
>
> On Tue, Apr 07, 2015 at 02:14:31PM -0700, Andy Zhou wrote:
>> assume av=1001, am = 1000, bv=1000, bm=1001.
>>
>> Should dv's last bit be zero? if not, then my interpretation of what
>> this function does
>> is wrong.
>>
>> On Mon, Apr 6, 2015 at 9:31 PM, Ben Pfaff <blp at nicira.com> wrote:
>> > On Mon, Apr 06, 2015 at 03:28:37PM -0700, Andy Zhou wrote:
>> >> On Tue, Mar 31, 2015 at 9:52 PM, Ben Pfaff <blp at nicira.com> wrote:
>> >> > To be first used in upcoming commits.
>> >> >
>> >> > Signed-off-by: Ben Pfaff <blp at nicira.com>
>> >>
>> >> > +/* Consider the two value/mask pairs 'a_value/a_mask' and 'b_value/b_mask' as
>> >> > + * restrictions on a field's value.  Then, this function initializes
>> >> > + * 'dst_value/dst_mask' such that it combines the restrictions of both pairs.
>> >> > + * This is not always possible, i.e. if one pair insists on a value of 0 in
>> >> > + * some bit and the other pair insists on a value of 1 in that bit.  This
>> >> > + * function returns false in a case where the combined restriction is
>> >> > + * impossible (in which case 'dst_value/dst_mask' is not fully initialized),
>> >> > + * true otherwise. */
>> >> > +bool
>> >> > +mf_subvalue_intersect(const union mf_subvalue *a_value,
>> >> > +                      const union mf_subvalue *a_mask,
>> >> > +                      const union mf_subvalue *b_value,
>> >> > +                      const union mf_subvalue *b_mask,
>> >> > +                      union mf_subvalue *dst_value,
>> >> > +                      union mf_subvalue *dst_mask)
>> >> > +{
>> >> > +    for (int i = 0; i < ARRAY_SIZE(a_value->be64); i++) {
>> >> > +        ovs_be64 av = a_value->be64[i];
>> >> > +        ovs_be64 am = a_mask->be64[i];
>> >> > +        ovs_be64 bv = b_value->be64[i];
>> >> > +        ovs_be64 bm = b_mask->be64[i];
>> >> > +        ovs_be64 *dv = &dst_value->be64[i];
>> >> > +        ovs_be64 *dm = &dst_mask->be64[i];
>> >> > +
>> >> > +        if ((av ^ bv) & (am & bm)) {
>> >> > +            return false;
>> >> > +        }
>> >> > +        *dv = av | bv;
>> >> > +        *dm = am | bm;
>> >> Should this be *dv = (av & am) | (bv & bm)?  I have not read the following patch
>> >> to check the actual usage, so my interpretation of 'intersect' may be
>> >> different than
>> >> what's intended here.
>> >
>> > I think that in the cases where the expression you suggest would have a
>> > different value, the "if" statement just before would have bailed out.



More information about the dev mailing list