On 6/2/22 16:28, Kevin Traynor wrote: > On 02/06/2022 15:00, Ilya Maximets wrote: >> On 6/2/22 13:01, Kevin Traynor wrote: >>> On 01/06/2022 14:30, David Marchand wrote: >>>> On Wed, May 25, 2022 at 1:11 PM Kevin Traynor <[email protected]> wrote: >>>>> >>>>> Currently mempools for vhost are being assigned before the vhost device >>>>> is added. In some cases this may be just reusing an existing mempool but >>>>> in others it can require creation of a mempool. >>>>> >>>>> For multi-NUMA, the NUMA info of the vhost port is not known until a >>>>> device is added to the port, so on multi-NUMA systems the initial NUMA >>>>> node for the mempool is a best guess based on vswitchd affinity. >>>>> >>>>> When a device is added to the vhost port, the NUMA info can be checked >>>>> and if the guess was incorrect a mempool on the correct NUMA node >>>>> created. >>>>> >>>>> For multi-NUMA, the current scheme can have the effect of creating a >>>>> mempool on a NUMA node that will not be needed and at least for a certain >>>>> time period requires more memory on a NUMA node. >>>>> >>>>> It is also difficult for a user trying to provision memory on different >>>>> NUMA nodes, if they are not sure which NUMA node the initial mempool >>>>> for a vhost port will be on. >>>>> >>>>> For single NUMA, even though the mempool will be on the correct NUMA, it >>>>> is assigned ahead of time and if a vhost device was not added, it could >>>>> also be using uneeded memory. >>>>> >>>>> This patch delays the creation of the mempool for a vhost port until the >>>>> vhost device is added. >>>>> >>>>> Signed-off-by: Kevin Traynor <[email protected]> >>>>> Reviewed-by: David Marchand <[email protected]> >>>> >>>> Thanks for aligning the behavior, and updating the commitlog. >>>> I did not test the mono numa part but I guess you did, and I don't see >>>> what could be wrong. >>>> >>>> I just wonder if we should add some note in NEWS for this change of >>>> behavior, though it is really low-level/internal... >>>> >>>> Regardless of the update on NEWS, this version lgtm. >>>> >>> >>> Thanks David. I don't mind about the NEWS. It is a low-level change, but it >>> might help a user to know if it is the new or old behaviour, in case they >>> are debugging memory problems, logs etc. How about: >>> >>> "Delay creating or reusing a mempool for vhost ports until the vhost device >>> has been added." >> >> I'm curious, does this change cause delayed error reporting >> or we do not report error on add-port already? >> > > Yeah, if it failed to get a mempool under both schemes, then the log would > appear at a later time now. That is part of what i was thinking about when i > mentioned debugging/logs. > >> I don't mind it be either way, it just maybe worth highlighting >> for users. >> > > Sure, I can add a short sentence to mention. e.g. > > "Delay creating or reusing a mempool for vhost ports until the vhost device > has been added. A failure to create a mempool will now also be logged when > the vhost device is added." > > Sounds ok?
"until the vhost device has been added" sounds a bit too formal, I guess. "until VM is started" would be more user-friendly. I know that this is not really correct (e.g. virtio-user), but should be more understandable? Also, I'm not sure if "vhost device" is a right term. "until the virtio device attached/connected" maybe? I mean, OVS registers a vhost device, but it's kind of an internal thing (OVS creates vhost device for itself), the event users are interested in is a client's virtio device connecting to our vhost-user port. Naming is confusing around vhost/virtio. I hope I got it right. :) Does that make any sense? Other suggestions are welcome. _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
