| 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. | expand |
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
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
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.)
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
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;
'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(-)