On 8/6/24 18:55, Mike Pattrick wrote: > On Tue, Aug 6, 2024 at 9:29 AM Ilya Maximets <[email protected]> wrote: >> >> On 7/24/24 20:07, Mike Pattrick wrote: >>> Currently it is possible to call netdev-dpdk/get-mempool-info before a >>> mempool as been created. This can happen because a device is added to >>> the netdev_shash before a mempool is allocated for it, which results in >>> a segmentation fault. >>> >>> Now we check for a NULL value before attempting to dereference it. >>> >>> Fixes: be4817331071 ("netdev-dpdk: Add debug appctl to get mempool >>> information.") >>> Signed-off-by: Mike Pattrick <[email protected]> >>> --- >>> v2: Added a test for this issue. >>> --- >>> lib/netdev-dpdk.c | 12 ++++++++---- >>> tests/system-dpdk.at | 5 +++++ >>> 2 files changed, 13 insertions(+), 4 deletions(-) >>> >>> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c >>> index 02cef6e45..6afa6da88 100644 >>> --- a/lib/netdev-dpdk.c >>> +++ b/lib/netdev-dpdk.c >>> @@ -4664,10 +4664,14 @@ netdev_dpdk_get_mempool_info(struct unixctl_conn >>> *conn, >>> ovs_mutex_lock(&dev->mutex); >>> ovs_mutex_lock(&dpdk_mp_mutex); >>> >>> - rte_mempool_dump(stream, dev->dpdk_mp->mp); >>> - fprintf(stream, " count: avail (%u), in use (%u)\n", >>> - rte_mempool_avail_count(dev->dpdk_mp->mp), >>> - rte_mempool_in_use_count(dev->dpdk_mp->mp)); >>> + if (dev->dpdk_mp) { >>> + rte_mempool_dump(stream, dev->dpdk_mp->mp); >>> + fprintf(stream, " count: avail (%u), in use (%u)\n", >>> + rte_mempool_avail_count(dev->dpdk_mp->mp), >>> + rte_mempool_in_use_count(dev->dpdk_mp->mp)); >>> + } else { >>> + fprintf(stream, " Not allocated\n"); >> >> Hi, Mike. Do we actually need these spaces at the beginning? >> The other fprintf has them because this is the way rte_mempool_dump() >> formats mempool statistics and we're trying to blend in, but it seems >> unnecessary for the no-mempool case. > > I don't know what is best here. The spaces are similar to the current > formatting,
The actual mempool dump starts from the beginning of the line without extra spaces. So, I'd not say it's similar. > but maybe nothing should be printed at all instead? One other option might be to return the same message, but as an error, i.e. with unixctl_command_reply_error(). > > -M > >> >> Best regards, Ilya Maixmets. >> >>> + } >>> >>> ovs_mutex_unlock(&dpdk_mp_mutex); >>> ovs_mutex_unlock(&dev->mutex); >>> diff --git a/tests/system-dpdk.at b/tests/system-dpdk.at >>> index 1c97bf777..ecf4c7496 100644 >>> --- a/tests/system-dpdk.at >>> +++ b/tests/system-dpdk.at >>> @@ -88,6 +88,11 @@ ADD_VHOST_USER_CLIENT_PORT([br10], >>> [dpdkvhostuserclient0], [$OVS_RUNDIR/dpdkvhos >>> AT_CHECK([ovs-vsctl show], [], [stdout]) >>> sleep 2 >>> >>> +dnl Check that no mempool was allocated. >>> +AT_CHECK([ovs-appctl netdev-dpdk/get-mempool-info dpdkvhostuserclient0], >>> [0], [dnl >>> + Not allocated >>> +]) >>> + >>> dnl Clean up >>> AT_CHECK([ovs-vsctl del-port br10 dpdkvhostuserclient0], [], [stdout], >>> [stderr]) >>> OVS_DPDK_STOP_VSWITCHD(["dnl >> > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
