On 6/3/22 11:19, Kevin Traynor wrote: > On 02/06/2022 17:10, Ilya Maximets wrote: >> 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. :) >> > > I think you proved the point that while not fully accurate, "until the VM is > started." is probably the best for understanding for most OVS users :-) > > Unless there's some better suggestion by the afternoon, I will resend with > that.
I'm OK with that. On the other note, " will now also be logged when" should probbaly be "will now also be logged only when", maybe even without "also". Otherwise it sounds like the error will be logged twice. What do you think? And, please, keep 2 empty lines between NEWS sections. :) > >> Does that make any sense? Other suggestions are welcome. >> > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
