From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([209.51.188.92]:60480) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ghccO-0006DP-IS for qemu-devel@nongnu.org; Thu, 10 Jan 2019 10:51:42 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ghcbO-00039S-FK for qemu-devel@nongnu.org; Thu, 10 Jan 2019 10:50:40 -0500 Received: from mx0b-001b2d01.pphosted.com ([148.163.158.5]:47360 helo=mx0a-001b2d01.pphosted.com) by eggs.gnu.org with esmtps (TLS1.0:RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1ghcbO-00037E-87 for qemu-devel@nongnu.org; Thu, 10 Jan 2019 10:50:38 -0500 Received: from pps.filterd (m0098416.ppops.net [127.0.0.1]) by mx0b-001b2d01.pphosted.com (8.16.0.22/8.16.0.22) with SMTP id x0AFil7O186344 for ; Thu, 10 Jan 2019 10:50:35 -0500 Received: from e15.ny.us.ibm.com (e15.ny.us.ibm.com [129.33.205.205]) by mx0b-001b2d01.pphosted.com with ESMTP id 2px7chwq4c-1 (version=TLSv1.2 cipher=AES256-GCM-SHA384 bits=256 verify=NOT) for ; Thu, 10 Jan 2019 10:50:35 -0500 Received: from localhost by e15.ny.us.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Thu, 10 Jan 2019 15:50:34 -0000 References: <1545062250-7573-1-git-send-email-akrowiak@linux.ibm.com> <112a03e6-3d6c-1c14-10ab-c9cea09ad388@linux.ibm.com> <20190108173113.3c2d830c.cohuck@redhat.com> <20190108175021.003deab9@oc2783563651> <20190108180615.7e48cf13.cohuck@redhat.com> <4876194e-1a87-1491-a8b3-eed5b0a9afe6@linux.ibm.com> <20190109111419.426ec6d7.cohuck@redhat.com> <20190109183538.05bab3c8@oc2783563651> From: Tony Krowiak Date: Thu, 10 Jan 2019 10:50:30 -0500 MIME-Version: 1.0 In-Reply-To: <20190109183538.05bab3c8@oc2783563651> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Message-Id: <04965ae7-da3d-a68a-ce0c-eb0b02e542ed@linux.ibm.com> Subject: Re: [Qemu-devel] [PATCH v3] qdev/core: fix qbus_is_full() List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Halil Pasic Cc: Cornelia Huck , imammedo@redhat.com, Peter Maydell , qemu-devel@nongnu.org, Christian Borntraeger On 1/9/19 12:35 PM, Halil Pasic wrote: > On Wed, 9 Jan 2019 10:36:11 -0500 > Tony Krowiak wrote: > >> On 1/9/19 5:14 AM, Cornelia Huck wrote: >>> On Tue, 8 Jan 2019 15:34:37 -0500 >>> Tony Krowiak wrote: >>> >>>> On 1/8/19 12:06 PM, Cornelia Huck wrote: >>>>> On Tue, 8 Jan 2019 17:50:21 +0100 >>>>> Halil Pasic wrote: >>>>> >>>>>> On Tue, 8 Jan 2019 17:31:13 +0100 >>>>>> Cornelia Huck wrote: >>>>>> >>>>>>> On Tue, 8 Jan 2019 11:08:56 -0500 >>>>>>> Tony Krowiak wrote: >>>>>>> >>>>>>>> On 12/17/18 10:57 AM, Tony Krowiak wrote: >>>>>>>>> The qbus_is_full(BusState *bus) function (qdev_monitor.c) compares the max_index >>>>>>>>> value of the BusState structure with the max_dev value of the BusClass structure >>>>>>>>> to determine whether the maximum number of children has been reached for the >>>>>>>>> bus. The problem is, the max_index field of the BusState structure does not >>>>>>>>> necessarily reflect the number of devices that have been plugged into >>>>>>>>> the bus. >>>>>>>>> >>>>>>>>> Whenever a child device is plugged into the bus, the bus's max_index value is >>>>>>>>> assigned to the child device and then incremented. If the child is subsequently >>>>>>>>> unplugged, the value of the max_index does not change and no longer reflects the >>>>>>>>> number of children. >>>>>>>>> >>>>>>>>> When the bus's max_index value reaches the maximum number of devices >>>>>>>>> allowed for the bus (i.e., the max_dev field in the BusClass structure), >>>>>>>>> attempts to plug another device will be rejected claiming that the bus is >>>>>>>>> full -- even if the bus is actually empty. >>>>>>>>> >>>>>>>>> To resolve the problem, a new 'num_children' field is being added to the >>>>>>>>> BusState structure to keep track of the number of children plugged into the >>>>>>>>> bus. It will be incremented when a child is plugged, and decremented when a >>>>>>>>> child is unplugged. >>>>>>>>> >>>>>>>>> Signed-off-by: Tony Krowiak >>>>>>>>> Reviewed-by: Pierre Morel >>>>>>>>> Reviewed-by: Halil Pasic >>>>>>>>> --- >>>>>>>>> hw/core/qdev.c | 3 +++ >>>>>>>>> include/hw/qdev-core.h | 1 + >>>>>>>>> qdev-monitor.c | 2 +- >>>>>>>>> 3 files changed, 5 insertions(+), 1 deletion(-) >>>>>>>> >>>>>>>> Ping ... >>>>>>>> I could not determine who the maintainer is for the three files >>>>>>>> listed above. I checked the MAINTAINERS file and the prologue of each >>>>>>>> individual file. Can someone please tell me who is responsible >>>>>>>> for merging these changes? Any additional review comments? >>>>>>>> >>>>>>>>> >>>>>>>>> diff --git a/hw/core/qdev.c b/hw/core/qdev.c >>>>>>>>> index 6b3cc55b27c2..956923f33520 100644 >>>>>>>>> --- a/hw/core/qdev.c >>>>>>>>> +++ b/hw/core/qdev.c >>>>>>>>> @@ -59,6 +59,8 @@ static void bus_remove_child(BusState *bus, DeviceState *child) >>>>>>>>> snprintf(name, sizeof(name), "child[%d]", kid->index); >>>>>>>>> QTAILQ_REMOVE(&bus->children, kid, sibling); >>>>>>>>> >>>>>>>>> + bus->num_children--; >>>>>>>>> + >>>>>>>>> /* This gives back ownership of kid->child back to us. */ >>>>>>>>> object_property_del(OBJECT(bus), name, NULL); >>>>>>>>> object_unref(OBJECT(kid->child)); >>>>>>>>> @@ -73,6 +75,7 @@ static void bus_add_child(BusState *bus, DeviceState *child) >>>>>>>>> char name[32]; >>>>>>>>> BusChild *kid = g_malloc0(sizeof(*kid)); >>>>>>>>> >>>>>>>>> + bus->num_children++; >>>>>>>>> kid->index = bus->max_index++; >>>>>>> >>>>>>> Hm... I'm wondering what happens for insane numbers of hotplugging >>>>>>> operations here? >>>>>>> >>>>>>> (Preexisting problem for busses without limit, but busses with a limit >>>>>>> could now run into that as well.) >>>>>>> >>>>>> >>>>>> How does this patch change things? I mean bus->num_children gets >>>>>> decremented on unplug. >>>>> >>>>> We don't stop anymore if max_index >= max_dev, which means that we can >>>>> now trigger that even if max_dev != 0. >>>> >>>> I guess I am missing your point. If max_dev == 0, then there is nothing >>>> stopping an insane number of hot plug operations; either before this >>>> patch, or with this patch. With the patch, once the number of children >>>> hot plugged reaches max_dev, the qbus_is_full function will return false >>>> and no more children can be plugged. If a child device is unplugged, >>>> the num_children - which counts the number of children attached to the >>>> bus - will be decremented, so it always reflects the number of children >>>> added to the bus. Besides, checking max_index against max_dev >>>> is erroneous, because max_index is incremented every time a child device >>>> is plugged and is never decremented. It really operates as more of a >>>> uinique identifier than a counter and is also used to create a unique >>>> property name when the child device is linked to the bus as a property >>>> (see bus_add_child function in hw/core/qdev.c). >>> >>> Checking num_children against max_dev is the right thing to do, no >>> argument here. >>> >>> However, max_index continues to be incremented even if devices have >>> been unplugged again. That means it can overflow, as it is never bound >>> by the max_dev constraint. >>> >>> This has been a problem before for busses with an unrestricted number of >>> devices before, but with your patch, the problem is now triggerable for >>> all busses. >>> >>> Not a problem with your patch, but we might want to look into making >>> max_index overflow/wraparound save. >> >> I see your point. It does beg the question, what are the chances that >> max_index reaches INT_MAX? In the interest of making this code more >> bullet proof, I suppose it is something that should be dealt with. >> >> A search reveals that max_index is used in only two places: It is used >> to set the index for a child of the bus (i.e., bus_add_child function in >> hw/core/qdev.c); and prior to this patch, to determine if max_dev has >> been exceeded (i.e., qbus_is_full function in qdev_monitor.c). From >> what I can see, the bus child index is used only to generate a property >> name of the format "child[%d]" so the child can be linked as a property >> of the bus (see bus_add_child and bus_remove_child functions in >> hw/core/qdev.c). Wraparound of the max_index would most likely result in >> generating a duplicate property name for the child. >> >> I propose two possible solutions: >> >> 1. When max_index reaches INT_MAX, do not allow any additional children >> to be added to the bus. >> >> 2. Set a max_dev value of INT_MAX for the BusClass instance if the value >> is not set (in the bus_class_init function in hw/core/bus.c). >> >> 3. Instead of generating the property name from the BusChild index >> value, generate a UUID string. Since the index field of the BusChild >> appears to be used only to generate the child's name, maybe change >> the index field to a UUID field or a name field. >> > > Separate problem, separate patch, or? Good question. > > UUID instead of index solves the problem of unique names I guess, but I > can't tell if that would be acceptable (interface stability, etc.). I may have missed something, but as currently used, the BusChild index is accessed in only two places; when the child is added to the bus and when it is removed from the bus. In both cases, it is used to derive a unique property name for the child device. Speaking of index, it implies ordering of the bus's children. IMHO, it only makes semantical sense if the index changes when a child device with a lower index value is removed from the bus. If a child device has an index of 5 - i.e., the fifth child on the bus - and the child device with index 4 is removed, then the child device with index 5 is no longer the fifth child on the bus. Maybe its just the fact that these fields are named 'index'. The fact that they are not really used as indexes caused a bit of confusion for me when analyzing this code and seems like a misnomer to me. > > The max_dev won't help because we can get a collision while maintaining > the invariant 'not more than 2 devices on the bus'. I don't understand, can you better explain what you mean? When you say "we can get a collision", are you talking about the property name? What does max_dev have to do with that? Please explain. What do you mean by "maintaining the invariant 'not more than 2 devices on the bus'"? > > So if we don't want to limit the number of hotplug operations, and we do > want to keep the allocation scheme before wrapping, the only solution I > see is looking for the first free identifier after we wrap. Are you are saying to look for the first index value that is not assigned to a BusChild object? > > BTW I wonder if making max_index and index unsigned make things bit less > ugly. I thought the same. They could also be made unsigned long or unsigned long long to increase the number of child devices that can be plugged in before having to deal with exceeding the index value. > > Regards, > Halil >