Re: [Xen-devel] [PATCH v7] x86/mem-sharing: mem-sharing a range of memory

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 19/07/16 17:27, Tamas K Lengyel wrote:
>
>>> +{
>>> +    int rc = 0;
>>> +    shr_handle_t sh, ch;
>>> +    unsigned long start =
>>> +        range->_scratchspace ? range->_scratchspace : range->start;
>> This can be shortened to "unsigned long start = range->_scratchspace ?:
>> range->start;" and fit on a single line.
> I'm not that familiar with this style of the syntax, does that have
> the effect of setting start = _scratchspace when _scratchspace is not
> 0?

It is a GCC extension
https://gcc.gnu.org/onlinedocs/gcc-6.1.0/gcc/Conditionals.html which
allows you to omit the middle parameter if it is identical to the first.

It is very useful for chaining together a load of items where you want
to stop at the first non-zero one.

x = a ?: b ?: c ?: d;

but can also be used with functions calls which 0 success, nonzero error
semantics:

rc = a() ?: b() ?: c() ?: d();

If you don't need to do any special cleanup in-between them.

>>> +    /*
>>> +     * We only propagate -ENOMEM as individual pages may fail with -EINVAL,
>>> +     * and for range sharing we only care if -ENOMEM was encountered so we 
>>> reset
>>> +     * rc here.
>>> +     */
>>> +    if ( rc < 0 && rc != -ENOMEM )
>> Would you mind putting in an ASSERT(rc == -EINVAL) here, if we believe
>> that to be an ok case to ignore?  In the future if more errors get
>> raised, we don't want to silently lose a more serious error which should
>> be propagated up.
> Well, in that case I can just change the if statement to rc == -EINVAL.

That is a much better suggestion.

>>> @@ -1468,6 +1520,94 @@ int 
>>> mem_sharing_memop(XEN_GUEST_HANDLE_PARAM(xen_mem_sharing_op_t) arg)
>>>          }
>>>          break;
>>>
>>> +        case XENMEM_sharing_op_range_share:
>>> +        {
>>> +            unsigned long max_sgfn, max_cgfn;
>>> +            struct domain *cd;
>>> +
>>> +            rc = -EINVAL;
>>> +            if( mso.u.range._pad[0] || mso.u.range._pad[1] ||
>>> +                mso.u.range._pad[2] )
>>> +                goto out;
>>> +
>>> +            /*
>>> +             * We use _scratchscape for the hypercall continuation value.
>>> +             * Ideally the user sets this to 0 in the beginning but
>>> +             * there is no good way of enforcing that here, so we just 
>>> check
>>> +             * that it's at least in range.
>>> +             */
>>> +            if ( mso.u.range._scratchspace &&
>>> +                (mso.u.range._scratchspace < mso.u.range.start ||
>>> +                 mso.u.range._scratchspace > mso.u.range.end) )
>> Alignment (extra space) for these two lines.
> You mean add an extra space or that there is an extra space?

Please add an extra space in.  It should look like:

if ( mso.u.range._scratchspace &&
     (mso.u.range._scratchspace ...
      mso.u.range._scratchspace ...

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel