[dpdk-dev] [PATCH 1/4] vhost: handle VHOST_USER_SET_LOG_BASE request

Panu Matilainen pmatilai at redhat.com
Wed Dec 2 17:58:03 CET 2015


On 12/02/2015 05:09 PM, Yuanhan Liu wrote:
> On Wed, Dec 02, 2015 at 04:48:14PM +0200, Panu Matilainen wrote:
> ...
>>>>> diff --git a/lib/librte_vhost/rte_virtio_net.h b/lib/librte_vhost/rte_virtio_net.h
>>>>> index 5687452..416dac2 100644
>>>>> --- a/lib/librte_vhost/rte_virtio_net.h
>>>>> +++ b/lib/librte_vhost/rte_virtio_net.h
>>>>> @@ -127,6 +127,8 @@ struct virtio_net {
>>>>>   #define IF_NAME_SZ (PATH_MAX > IFNAMSIZ ? PATH_MAX : IFNAMSIZ)
>>>>>   	char			ifname[IF_NAME_SZ];	/**< Name of the tap device or socket path. */
>>>>>   	uint32_t		virt_qp_nb;	/**< number of queue pair we have allocated */
>>>>> +	uint64_t		log_size;	/**< Size of log area */
>>>>> +	uint8_t			*log_base;	/**< Where dirty pages are logged */
>>>>>   	void			*priv;		/**< private context */
>>>>>   	struct vhost_virtqueue	*virtqueue[VHOST_MAX_QUEUE_PAIRS * 2];	/**< Contains all virtqueue information. */
>>>>>   } __rte_cache_aligned;
>>>>
>>>> This (and other changes in patch 2 breaks the librte_vhost ABI
>>>> again, so you'd need to at least add a deprecation note to 2.2 to be
>>>> able to do it in 2.3 at all according to the ABI policy.
>>>
>>> I was thinking that adding a new field (instead of renaming it or
>>> removing it) isn't an ABI break. So, I was wrong?
>>
>> Adding or removing a field in the middle of a public struct is
>> always an ABI break. Adding to the end often is too, but not always.
>> Renaming a field is an API break but not an ABI break - the compiler
>> cares but the cpu does not.
>
> Good to know. Thanks.
>
>>
>>>>
>>>> Perhaps a better option would be adding some padding to the structs
>>>> now for 2.2 since the vhost ABI is broken there anyway. That would
>>>> at least give a chance to keep it compatible from 2.2 to 2.3.
>>>
>>> It will not be compatible, unless we add exact same fields (not
>>> something like uint8_t pad[xx]). Otherwise, the pad field renaming
>>> is also an ABI break, right?
>>
>> There's no ABI (or API) break in changing reserved unused fields to
>> something else, as long as care is taken with sizes and alignment.
>
> as long as we don't reference the reserved unused fields?

That would be the definition of an unused field I think :)
Call it "reserved" if you want, it doesn't really matter as long as its 
clear its something you shouldn't be using.

>
>> In any case padding is best added to the end of a struct to minimize
>> risks and keep things simple.
>
> The thing is that isn't it a bit aweful to (always) add pads to
> the end of a struct, especially when you don't know how many
> need to be padded?

Then you pad for what you think you need, plus a bit extra, and maybe 
some more for others who might want to extend it. What is a reasonable 
amount needs deciding case by case - if a struct is alloced in the 
millions then be (very) conservative, but if there are one or 50 such 
structs within an app lifetime then who cares if its bit larger?

And yeah padding may be annoying, but that's pretty much the only option 
in a project where most of the structs are out in the open.

	- Panu -

>
> 	--yliu
>



More information about the dev mailing list