Re: [Xen-devel] [PATCH for-xen-4.5] libxl: Allow copying smaller bitmap into a larger one

[Wei Liu <wei.liu2@xxxxxxxxxx>]

On Mon, Nov 24, 2014 at 10:48:19AM -0500, Boris Ostrovsky wrote:
> On 11/24/2014 05:47 AM, Wei Liu wrote:
> >CC'ing Dario...
> >
> >On Mon, Nov 24, 2014 at 10:41:27AM +0000, Wei Liu wrote:
> >>On Thu, Nov 20, 2014 at 04:27:34PM -0500, Boris Ostrovsky wrote:
> >>>When parsing bitmap objects JSON parser will create libxl_bitmap
> >>>map of the smallest size needed.
> >>>
> >>>This can cause problems when saved image file specifies CPU affinity.
> >>>For example, if 'vcpu_hard_affinity' in the saved image has only the
> >>>first CPU specified, just a single byte will be allocated and
> >>>libxl_bitmap->size will be set to 1.
> >>>
> >>>This will result in assertion in 
> >>>libxl_set_vcpuaffinity()->libxl_bitmap_copy()
> >>>since the destination bitmap is created for maximum number of CPUs.
> >>>
> >>>We could allocate that bitmap of the same size as the source, however,
> >>>it is later passed to xc_vcpu_setaffinity() which expects it to be
> >>>sized to the max number of CPUs
> >>>
> >>>Instead, we should allow copying the (smaller) bitmap read by the parser
> >>>and keep the rest of bytes in the destination map unmodified (zero in
> >>>this case)
> >>>
> >>Here is some more thoughts on this issue:
> >>
> >>This API is used by VCPU placement and NUMA placement logic in libxl. To
> >>fix the breakage, we don't necessary need to expose new API or change
> >>API behaviour, we only need to have a internal function to do it.
> >>
> >>The reversed case (large bitmap to small one) is not valid in libxl, as
> >>in the pinning will fail. But bitmap copy happens before that, we would
> >>still need to deal with that. Dario, can you provide some input on
> >>the expected behaviour?
> 
> I understand that hypervisor will ignore bits that are beyond what it knows
> about --- see xenctl_bitmap_to_bitmap(). And libxl will will issue a
> warning. But only if byte-sized bitmaps are the same. If they are not will
> will pop the assertion (in debug builds).
> 

After a discussion on IRC with Dario, migrating from large host to small
one is legit use case. That means we need to take care about the
reversed case as well.

BTW, are you on xen-devel@freenode? It would be much faster if we can
discuss on IRC.

> >>
> >>The partial copy function should explicitly zero-out all remaining bits.
> 
> I actually thought that partial copy function should do just that --- copy
> bits that it has and leave others unchanged. The caller, if desires so,
> should have cleared the mask prior to the call. (This is for the case when
> destination is larger than source, of course).
> 

This is OK as long as this behaviour is documented.

I will prepare a patch shortly for you to test.

Wei.

> 
> -boris

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