From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 3FA8A45945; Mon, 9 Sep 2024 16:20:53 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id E34714028A; Mon, 9 Sep 2024 16:20:52 +0200 (CEST) Received: from mail-lf1-f44.google.com (mail-lf1-f44.google.com [209.85.167.44]) by mails.dpdk.org (Postfix) with ESMTP id 9D31840268 for ; Mon, 9 Sep 2024 16:20:51 +0200 (CEST) Received: by mail-lf1-f44.google.com with SMTP id 2adb3069b0e04-5344ab30508so5014313e87.0 for ; Mon, 09 Sep 2024 07:20:51 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=iol.unh.edu; s=unh-iol; t=1725891651; x=1726496451; darn=dpdk.org; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:from:to:cc:subject:date :message-id:reply-to; bh=9X1mWnSuGJlffOXdjkt3cGP+B4ZmwxwsALk1YYW0lyA=; b=VsecTWOpCO9utXIIiU8wklBK19pULxBLx6ko48UwHZtNOBwXv0ICiuIWY0k3LwIJIC vwVheiv1yUJuw2+uEXAeNBBprCFg80Y6mlVXZZoOGLVm9ruRU65uRiODAkpX4fNcfL+V B4q4PmiQeFfBzF96RxNo1C0SExsO5jV8XADiY= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1725891651; x=1726496451; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=9X1mWnSuGJlffOXdjkt3cGP+B4ZmwxwsALk1YYW0lyA=; b=xGbkD4ikYmHs30RvcRu9phur6RbxMWu9ax6wAsPf7SzJVg+DFC41u1l8otrq/UMrWB 9p98UQGsna6K9BQ0V+HG+MHgrCf6OnilwahMIfcX+sIxJjk+7cEPFomqEjCEjQqX/X9y YjFlKiVYmt/zs+DhDHE4/2EF/P0XdReV4/RPScRb0R12eF3+8xOqLUZg4T3cfVNIANuP Eg4wozjb2tfIHTblrWuu+iJuf5USnVwgtPet7FyrRibwR5aL2Gg/+g1n/YNRxsKRwFz0 2jdyA6YTWm/PMLcLerdtg0ejkH+x9VTLWCYGH8wPHKjO/dn/tpDOwlzTqGT97Fz95oE+ pW0g== X-Forwarded-Encrypted: i=1; AJvYcCXHgH+hkmsUKAp8LUvg7jkyXviys8ua3emZ1Gh6JOi6431ZmER76+VrQOKzuaMcXryjMSg=@dpdk.org X-Gm-Message-State: AOJu0YxJDNLT/xGwjLfkTR4uNnkBhLouRwbca7oDurIXki5tfqMCjYDO 57wfeyo2PTrqanWBmcUD0zrb8gHWrqe3lEBJtwW6sOKwehok8x3WeL4AE4hacJAZZaIO8oxzfpN BwuVdWLcbtZenK9mafP1+XKPa74Y9+tkDsNRx4w== X-Google-Smtp-Source: AGHT+IFRis2edjCzybRz0oLM9Pz+rZWXPxruJ0XcxuySOp3+qf6Z/Jd+bdS83++E2gwRuSlFUGTlTVSipvsLSIa+Lcc= X-Received: by 2002:a05:6512:b1e:b0:52f:413:7e89 with SMTP id 2adb3069b0e04-53657fcd3a2mr4446954e87.17.1725891650214; Mon, 09 Sep 2024 07:20:50 -0700 (PDT) MIME-Version: 1.0 References: <20240806121417.2567708-1-Luca.Vizzarro@arm.com> <20240806124642.2580828-1-luca.vizzarro@arm.com> <20240806124642.2580828-5-luca.vizzarro@arm.com> <89d39e22-ca0a-43a9-bc42-6c833a7cd8e4@pantheon.tech> <6b1e1e14-41f2-4303-99bf-24d95c1ed600@arm.com> In-Reply-To: <6b1e1e14-41f2-4303-99bf-24d95c1ed600@arm.com> From: Jeremy Spewock Date: Mon, 9 Sep 2024 10:20:35 -0400 Message-ID: Subject: Re: [PATCH v2 4/5] dts: add ability to start/stop testpmd ports To: Luca Vizzarro Cc: =?UTF-8?Q?Juraj_Linke=C5=A1?= , dev@dpdk.org, Honnappa Nagarahalli , Paul Szczepanek Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org On Fri, Sep 6, 2024 at 12:46=E2=80=AFPM Luca Vizzarro wrote: > > On 23/08/2024 13:16, Juraj Linke=C5=A1 wrote: > > As Jeremy mentioned, adding the verify argument may be worthwhile, but > > maybe only if we actually identify a usecase where we wouldn't want to > > do the verification. > > Yeah, as I pointed out, it feels unlikely to pretend that they are > started (or stopped). I think that could cause my unpredicted behaviour > and confusion. But also as said already, it can always be added on demand= . > I agree that we can save this as something to add as we need it and that there could be some confusion to pretending the ports are in a certain state. Since we actually save the state in testpmd, we definitely want that to be accurate. The main thing I was thinking about this for however was less about the state tracking and more about what the expectation is for developers when they specifically specify they do not want a method to be verified. There aren't many cases where you wouldn't want to verify the outcome of a command, but I would imagine the two reasons you would do so is either you don't care about the result, or you specifically don't want to be disruptive to the rest of the run if this command fails (like if the framework has a testpmd shell running for some kind of process but doesn't want to end the run if it fails). When we enforce the verification of some part of the process like stopping the port in this case, that assumption about not verifying changes. The second reason I mentioned is why I believe capabilities checking specifies no-verify when it updates the MTU, for example. I imagine we don't want the test run to end if a testpmd method fails its check with capabilities, we would want the capability to just be unsupported. Of course the developer could catch the exception themselves, but in that case I think it would make more sense for catching the exception to be the primary way to avoid verification and we could just remove the parameter all together so there isn't this edge-case for avoiding verification with decorated methods. I guess even the static parameter that specifies whether or not to verify doesn't really address this problem either, haha. Maybe it would even make more sense for the decorator to assume that it shouldn't throw an exception if the ports state changing failed (but still avoid updating the port state) and let the verifying process be completely down to if the method being decorated succeeded or not with the ports in whatever state they were at the time since, I would assume, if the ports fail to update their state the method will also fail anyway. All-in-all however, there likely won't be many cases where the ports actually fail to stop in the first place, and not verifying in general is uncommon. If what I mentioned above makes sense and we're fine with the slight change in thinking, I think it is fine to merge this and just keep in the back of our minds that this is the case, and maybe either document somewhere that you have to catch the exception with decorated methods, or just remove the verify parameter from decorated methods all together and enforce that they must be verified. As mentioned, it's not like we are forcing ourselves to stick with it, adding the parameter is pretty easy to do on demand. > > I also have two other points: > > 1. Do we want a decorator that does both - starting and stopping? Is th= e > > idea to have all methods that require a certain state to be decorated > > and in that case we don't need this? E.g. if there are multiple > > configuration steps, only the first one stops the ports (if started) an= d > > we start the ports only when a method needs that (such as starting pkt > > fwd)? I think this workflow should be documented in the class docstring > > (the important part being that starting and stopping of ports is done > > automatically). > > The way I envisioned these decorators is by using a lazy approach. I > don't think it may be right to eagerly restart ports after stopping > them, because maybe we want to run another command after that will stop > them again. So only start and stop as needed. Ideally every port that > requires a specific state of the ports need to be decorated. I went > through all the methods in the class to check which would and which > wouldn't, and it seems alright like this. > > > 2. Do we want decorators that start/stop just one port? I guess this is > > basically useless with the above workflow. > > Would it actually matter? We are not really using the other ports in > parallel here I believe. May bring unnecessary complexity. I am thinking > that maybe if needed it could be added later.