[ovs-dev] [PATCH 2/2] netdev-dpdk: Allocate vhost_id dynamically.

[Ian Stokes]

On 4/10/2019 5:50 PM, Ben Pfaff wrote:
> On Wed, Apr 10, 2019 at 05:35:34PM +0100, Ian Stokes wrote:
>> On 4/10/2019 5:21 PM, Ian Stokes wrote:
>>> On 3/5/2019 4:28 PM, Ilya Maximets wrote:
>>>> 'vhost_id' is an array of 'PATH_MAX' bytes in the middle of
>>>> 'netdev_dpdk' structure. That is 4K bytes.
>>>>
>>>> 'vhost_id' never used on a hot path and there is no need to keep
>>>> it inside the structure memory. Dynamic allocation will allow to
>>>> decrease 'struct netdev_dpdk' signigficantly, saving 4KB per ETH
>>> signigficantly -> significantly
>>>> port (ETH ports doesn't use 'vhost_id') and almost same value per
>>>> vhost ports (real 'vhost_id's, in common case, are much shorter).
>>>> We could save the pointer space by making the union with 'devargs'
>>>> which is mutually exclusive with 'vhost_id'.
>>>> As we're just removing the single 'PADDED_MEMBER', the total
>>>> cacheline layout is not affected.
>>>>
>>>> Stats for 'struct netdev_dpdk':
>>>>
>>>>       Before: /* size: 4992, cachelines: 78 */
>>>>       After : /* size:  896, cachelines: 14 */
>>>>
>>>
>>> Thanks Ilya, this looks like a good change, validated without issue.
>>>
>>> I've addressed a minor typo in the commit message and comment style
>>> issue below. Other than that it looks good so applied to master.
>>>
>>> On a broader note, do you think it's worth capturing changes such as
>>> this for users in documentation? i.e. previously the vhost-id path was
>>> limited to 4096 (however unlikely it was that someone would exceed
>>> this), but it wasn't captured in the OVS documentation from what I can
>>> see. From a usability point is it something to document between the 2.10
>>> and 2.11 release?
>>>
>>
>> Sorry, meant OVS 2.11 to 2.12 above.
> 
> I like to document user-visible changes, but I think in this case it is
> unlikely to fix any real user problems.  (It would be different if we
> made this change in response to a user having a problem with a very long
> vhost-id path.)
> 

Sure, I don't think a user would be aware of this unless they were 
hitting the 4096 limit, in which case I think they would have a 
different issue at hand :). It was just an after thought during the 
review when I checked if the vhost-user path limit was documented. Not 
necessary to be captured now though.

Ian