[ovs-dev,2/2] netdev-dpdk: Allocate vhost_id dynamically.
diff mbox series

Message ID 20190305162827.9059-3-i.maximets@samsung.com
State Accepted
Delegated to: Ian Stokes
Headers show
Series
  • netdev-dpdk: Dynamic allocation of vhost_id.
Related show

Commit Message

Ilya Maximets March 5, 2019, 4:28 p.m. UTC
'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
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 */

Signed-off-by: Ilya Maximets <i.maximets@samsung.com>
---
 lib/netdev-dpdk.c | 39 ++++++++++++++++++++++-----------------
 1 file changed, 22 insertions(+), 17 deletions(-)

Comments

Ian Stokes April 10, 2019, 4:21 p.m. UTC | #1
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?

Ian
Ian Stokes April 10, 2019, 4:35 p.m. UTC | #2
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.

Ian
Ben Pfaff April 10, 2019, 4:50 p.m. UTC | #3
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.)
Ian Stokes April 10, 2019, 5:05 p.m. UTC | #4
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

Patch
diff mbox series

diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
index 93d8e37bb..e5f9a9374 100644
--- a/lib/netdev-dpdk.c
+++ b/lib/netdev-dpdk.c
@@ -400,7 +400,12 @@  struct netdev_dpdk {
         enum dpdk_dev_type type;
         enum netdev_flags flags;
         int link_reset_cnt;
-        char *devargs;  /* Device arguments for dpdk ports */
+        union {
+            /* Device arguments for dpdk ports */
+            char *devargs;
+            /* Identifier used to distinguish vhost devices from each other. */
+            char *vhost_id;
+        };
         struct dpdk_tx_queue *tx_q;
         struct rte_eth_link link;
     );
@@ -417,11 +422,6 @@  struct netdev_dpdk {
         /* 3 pad bytes here. */
     );
 
-    PADDED_MEMBERS(CACHE_LINE_SIZE,
-        /* Identifier used to distinguish vhost devices from each other. */
-        char vhost_id[PATH_MAX];
-    );
-
     PADDED_MEMBERS(CACHE_LINE_SIZE,
         struct netdev up;
         /* In dpdk_list. */
@@ -1276,8 +1276,7 @@  netdev_dpdk_vhost_construct(struct netdev *netdev)
     /* Take the name of the vhost-user port and append it to the location where
      * the socket is to be created, then register the socket.
      */
-    snprintf(dev->vhost_id, sizeof dev->vhost_id, "%s/%s",
-             dpdk_get_vhost_sock_dir(), name);
+    dev->vhost_id = xasprintf("%s/%s", dpdk_get_vhost_sock_dir(), name);
 
     dev->vhost_driver_flags &= ~RTE_VHOST_USER_CLIENT;
     err = rte_vhost_driver_register(dev->vhost_id, dev->vhost_driver_flags);
@@ -1323,6 +1322,11 @@  netdev_dpdk_vhost_construct(struct netdev *netdev)
     }
 
 out:
+    if (err) {
+        free(dev->vhost_id);
+        dev->vhost_id = NULL;
+    }
+
     ovs_mutex_unlock(&dpdk_mutex);
     VLOG_WARN_ONCE("dpdkvhostuser ports are considered deprecated;  "
                    "please migrate to dpdkvhostuserclient ports.");
@@ -1451,13 +1455,14 @@  netdev_dpdk_vhost_destruct(struct netdev *netdev)
                  "socket '%s' must be restarted.", dev->vhost_id);
     }
 
-    vhost_id = xstrdup(dev->vhost_id);
+    vhost_id = dev->vhost_id;
+    dev->vhost_id = NULL;
 
     common_destruct(dev);
 
     ovs_mutex_unlock(&dpdk_mutex);
 
-    if (!vhost_id[0]) {
+    if (!vhost_id) {
         goto out;
     }
 
@@ -1906,8 +1911,9 @@  netdev_dpdk_vhost_client_set_config(struct netdev *netdev,
     ovs_mutex_lock(&dev->mutex);
     if (!(dev->vhost_driver_flags & RTE_VHOST_USER_CLIENT)) {
         path = smap_get(args, "vhost-server-path");
-        if (path && strcmp(path, dev->vhost_id)) {
-            strcpy(dev->vhost_id, path);
+        if (!nullable_string_is_equal(path, dev->vhost_id)) {
+            free(dev->vhost_id);
+            dev->vhost_id = nullable_xstrdup(path);
             /* check zero copy configuration */
             if (smap_get_bool(args, "dq-zero-copy", false)) {
                 dev->vhost_driver_flags |= RTE_VHOST_USER_DEQUEUE_ZERO_COPY;
@@ -3484,8 +3490,8 @@  new_device(int vid)
     /* Add device to the vhost port with the same name as that passed down. */
     LIST_FOR_EACH(dev, list_node, &dpdk_list) {
         ovs_mutex_lock(&dev->mutex);
-        if (strncmp(ifname, dev->vhost_id, IF_NAME_SZ) == 0) {
-            uint32_t qp_num = rte_vhost_get_vring_num(vid)/VIRTIO_QNUM;
+        if (nullable_string_is_equal(ifname, dev->vhost_id)) {
+            uint32_t qp_num = rte_vhost_get_vring_num(vid) / VIRTIO_QNUM;
 
             /* Get NUMA information */
             newnode = rte_vhost_get_numa_node(vid);
@@ -3613,7 +3619,7 @@  vring_state_changed(int vid, uint16_t queue_id, int enable)
     ovs_mutex_lock(&dpdk_mutex);
     LIST_FOR_EACH (dev, list_node, &dpdk_list) {
         ovs_mutex_lock(&dev->mutex);
-        if (strncmp(ifname, dev->vhost_id, IF_NAME_SZ) == 0) {
+        if (nullable_string_is_equal(ifname, dev->vhost_id)) {
             if (enable) {
                 dev->tx_q[qid].map = qid;
             } else {
@@ -4144,8 +4150,7 @@  netdev_dpdk_vhost_client_reconfigure(struct netdev *netdev)
      *  1. Device hasn't been registered yet.
      *  2. A path has been specified.
      */
-    if (!(dev->vhost_driver_flags & RTE_VHOST_USER_CLIENT)
-            && strlen(dev->vhost_id)) {
+    if (!(dev->vhost_driver_flags & RTE_VHOST_USER_CLIENT) && dev->vhost_id) {
         /* Register client-mode device. */
         vhost_flags |= RTE_VHOST_USER_CLIENT;