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 AE3B3454B7; Wed, 26 Jun 2024 17:55:37 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 45340402CF; Wed, 26 Jun 2024 17:55:37 +0200 (CEST) Received: from mail-pj1-f43.google.com (mail-pj1-f43.google.com [209.85.216.43]) by mails.dpdk.org (Postfix) with ESMTP id 75A7E402CC for ; Wed, 26 Jun 2024 17:55:35 +0200 (CEST) Received: by mail-pj1-f43.google.com with SMTP id 98e67ed59e1d1-2c85ca2dc5cso585254a91.0 for ; Wed, 26 Jun 2024 08:55:35 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=iol.unh.edu; s=unh-iol; t=1719417334; x=1720022134; 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=ihcJrPYRVZMbij+mDaT6M3ksDab2f9gZuCOuT45C6IY=; b=KqV++F52uqQvJihpabPkE0IPhfZcwpMt9UxtkTupJMq4z2I2DXNi08ZtcSSSyLpks7 KAP/UyKGQIK40xG4wFrFLFGznBgbv+7etzwUA/aUMqc1XxSPwaUCOuZOKBOaXm5NP15M i+lj75e9/dsXZZtWd8w48Jw6liwOoYo0HdhDA= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1719417334; x=1720022134; 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=ihcJrPYRVZMbij+mDaT6M3ksDab2f9gZuCOuT45C6IY=; b=UsLz62ID7q4VURUOBrFUVTDH4R6Tcqnm/DVm/iKC7PyFdgmG2dJIMjSUQDq3rv9zw2 OeR+3l0nCcNHYEDwslfT5VUqyTS6L+afdKK8ae8kPUdgOd8Ihm06AnfNEHrVDnGcYu1p sQnKW3Fb6QBIpjI7LE01zYuC9O+GtvdEmM4bKzAZtkzMLQ9ZLKes9j8wMMCE24waIis0 8qrufIRys50k9tDvrjn+786eADxfTr+6F9MpNmwZZnvNm0paAYd9QsHtg0r870Mrtl0V KyarcoNfqOUfNR3ge8xbuJPf2+CT88kNFx4jTSvEBY7vI7AzL+co9ZsK3ppJXqYzfhap pBAw== X-Forwarded-Encrypted: i=1; AJvYcCV3lhhgRF2BKXvcreISfdhrOH2DOBQZDyhNfKlLw48GzXrOoeLgY4/bdTmYPwBZUYXPVSz7lvFXc38W1Z4= X-Gm-Message-State: AOJu0YxkWuhUTK6Gye4TU59xrRJwXs1//YG0AxYZzwWxc8xhjU1cx28Y okHmTcAOVbLFbXfzqtOldbtCcUnf6V9ReaOdfElzPApq4jg37tDsxs+YWrX1pZ3JDO+OqF1KgrP 7uXvFuQF2OeTMskFCjqyEZtWD/PJXSMdPiOYo8g== X-Google-Smtp-Source: AGHT+IECM0DxcOa2yIJ00zGfBChtZFdIGppPIIW4T2X2gl8tPaB1VTDBenDRZwdPPmlnkZrjRvl1yKfycoGoQO0JkLs= X-Received: by 2002:a17:90b:1086:b0:2c2:c3f5:33c3 with SMTP id 98e67ed59e1d1-2c8489d761bmr15573753a91.6.1719417334262; Wed, 26 Jun 2024 08:55:34 -0700 (PDT) MIME-Version: 1.0 References: <20240621172059.8194-2-npratte@iol.unh.edu> <20240621172059.8194-8-npratte@iol.unh.edu> In-Reply-To: <20240621172059.8194-8-npratte@iol.unh.edu> From: Jeremy Spewock Date: Wed, 26 Jun 2024 11:55:23 -0400 Message-ID: Subject: Re: [PATCH 3/3] dts: mac filter test suite refactored for new dts To: Nicholas Pratte Cc: juraj.linkes@pantheon.tech, paul.szczepanek@arm.com, probb@iol.unh.edu, Honnappa.Nagarahalli@arm.com, yoan.picchi@foss.arm.com, dmarx@iol.unh.edu, luca.vizzarro@arm.com, bruce.richardson@intel.com, dev@dpdk.org 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, Jun 21, 2024 at 1:24=E2=80=AFPM Nicholas Pratte wrote: > > The mac address filter test suite, whose test cases are based on old > DTS's test cases, has been refactored to interface with the new DTS > framework. > > In porting over this test suite into the new framework, some > adjustments were made, namely in the EAL and TestPMD parameter provided > before executing the application. While the original test plan was > referenced, by and large, only for the individual test cases, I'll leave > the parameters the original test plan was asking for below for the sake > of discussion: > > --burst=3D1 --rxpt=3D0 --rxht=3D0 --rxwt=3D0 --txpt=3D36 --txht=3D0 --txw= t=3D0 > --txfreet=3D32 --rxfreet=3D64 --mbcache=3D250 --portmask=3D0x3 > > Bugzilla ID: 1454 > Signed-off-by: Nicholas Pratte > --- > + def send_packet_and_verify( > + self, > + mac_address: str, > + add_vlan: bool =3D False, > + should_receive: bool =3D True, > + ) -> None: > + """Generate, send, and verify a packet based on specified parame= ters. > + > + Test cases within this suite utilize this method to create, send= , and verify > + packets based on criteria relating to the packet's destination m= ac address, > + vlan tag, and whether or not the packet should be received or no= t. Packets > + are verified using an inserted payload. If the list of received = packets > + contains this payload within any of its packets, the test case p= asses. This > + implementation does not require a quiet wire, each call with thi= s method I don't think we need to mention that it doesn't require a quiet wire, in general this is something we are writing all test cases to assume. Looking at this though, it would probably be a great thing to add to the documentation for writing test suites. We never really mention anywhere that you must write test cases without this assumption, it's sort of just something we decided. Of course out of scope of this patch, but something for the future. > + sends exactly one packet. > + > + Args: > + mac_address: The destination mac address of the packet being= sent. > + add_vlan: Add a vlan tag to the packet being sent. :data:'2'= if the packet > + should be received, :data:'1' if the packet should not b= e received but > + requires a vlan tag, and None for any other condition. This confused me a little because I thought you were saying this parameter would be 2 if the packet should be received or it would be 1 in another case. I think this parameter is really just denoting if we should add a VLAN tag at all, and then the should_receive specifies what the tag will be. We should probably move the information about either being 2 or 1 to the `should_receive` documentation and specify that the VLAN tag will be that value based on if it should be received. This is a little hard because it relies on both conditions, but you can just say in the `should_recieve` parameter that, if there is a vlan tag, this is what it will be. > + should_receive: If :data:'True', assert whether or not the s= ent packet > + has been received. If :data:'False', assert that the sen= d packet was not > + received. :data:'True' by default > + """ > + packet =3D ( > + Ether() > + / Dot1Q(vlan=3D2 if add_vlan and should_receive else 1 if ad= d_vlan else None) I haven't used the Dot1Q class in scapy very much myself, but it seems like just setting vlan=3DNone is strange. Does this just make it completely omit the VLAN tag? Also, if we don't have a VLAN tag, should we just not include the Dot1Q layer at all? It feels like it doesn't really do anything if you don't have a VLAN. > + / IP() > + / Raw(load=3D"P" * 22) > + ) > + packet.dst =3D mac_address > + received_packets =3D [ > + packets > + for packets in self.send_packet_and_capture(packet, adjust_a= ddresses=3DFalse) > + if hasattr(packets, "load") and "P" * 22 in str(packets.load= ) > + ] i think this would be a little easier using the filter() built-in Python function. It would instead be: recieved_packets =3D [filter(lambda x: if hasattr(x, "load") and "P" * 22 in str(x.load), self.send_packet_and_capture(packet, adjust_addresses=3DFalse)] It doesn't really save you that much verbosity now that I've written it out, you kind of trade the "for x in ..." for "filter(lambda x: ..." so maybe it isn't any more valuable. However, this function for if a packet has a certain payload is becoming more and more common, I wonder if we could just add a method to test_suite called packet_has_payload(p: Packet, payload: str) that just checks does the packet have the load attribute and does that load attribute match the desired payload. Or, even better, if we made it a high-order function that just accepts a payload and then returns a function that validates if a packet has that payload. Something like this: def packet_has_payload(payload: str) -> Callable[[Packet], bool]: def wrap(p: Packet) -> bool: return if hasattr(p, "load") and payload in str(p.load) return wrap If we had this high-order function then filtering becomes super easy. The filter would just become: recieved_packets =3D [filter(self.packet_has_payload("P" * 22), self.send_packet_and_capture(packet, adjust_addresses=3DFalse)] Maybe this is something that should be addressed in the bugzilla ticket 1372 however. > + if should_receive: > + self.verify(len(received_packets) =3D=3D 1, "Expected packet= not received") > + else: > + self.verify(len(received_packets) =3D=3D 0, "Expected packet= received") > + > + def test_add_remove_mac_addresses(self) -> None: > + """Assess basic mac addressing filtering functionalities. > + > + This test cases validates for proper behavior of mac address fil= tering with both > + a port's default, burned-in mac address, as well as additional m= ac addresses > + added to the PMD. Packets should either be received or not recei= ved depending on > + the properties applied to the PMD at any given time. > + > + Test: > + Start TestPMD with promiscuous mode. > + Send a packet with the port's default mac address. (Should r= eceive) > + Send a packet with fake mac address. (Should not receive) > + Add fake mac address to the PMD's address pool. > + Send a packet with the fake mac address to the PMD. (Should = receive) > + Remove the fake mac address from the PMD's address pool. > + Sent a packet with the fake mac address to the PMD. (Should = not receive) Typo: Sent a packet. > + """ > + testpmd =3D TestPmdShell(self.sut_node) > + testpmd.set_promisc(0, on=3DFalse, verify=3DFalse) Why don't we want to verify this command? If there is a reason we should probably leave a comment explaining it. > + testpmd.start() > + mac_address =3D self._sut_port_ingress.mac_address > + > + # Send a packet with NIC default mac address > + self.send_packet_and_verify(mac_address=3Dmac_address, should_re= ceive=3DTrue) > + # Send a packet with different mac address > + fake_address =3D "00:00:00:00:00:01" > + self.send_packet_and_verify(mac_address=3Dfake_address, should_r= eceive=3DFalse) > + > + # Add mac address to pool and rerun tests > + testpmd.set_mac_addr(0, mac_address=3Dfake_address, verify=3DTru= e) verify=3DTrue if the default and we generally omit this, we should probably omit it here as well. > + self.send_packet_and_verify(mac_address=3Dfake_address, should_r= eceive=3DTrue) > + testpmd.set_mac_addr(0, mac_address=3Dfake_address, add=3DFalse) > + self.send_packet_and_verify(mac_address=3Dfake_address, should_r= eceive=3DFalse) > + > + def test_invalid_address(self) -> None: > + """Assess the behavior of a NIC mac address pool while binded to= the PMD. Should this be binded or bound? I think it's bound, but I'm not sure if it's different when referring to drivers. > + > + An assessment of a NIC's behavior when mounted to a PMD as it re= lates to mac addresses > + and address pooling. Devices should not be able to use invalid m= ac addresses, remove their > + built-in hardware address, or exceed their address pools. > + > + Test: > + Start TestPMD. > + Attempt to set add an invalid mac address. (Should fail) Typo: Attempt to set add an invalid address. > + Attempt to remove the device's hardware address. (Should fai= l) > + Add a fake mac address to the pool twice in succession. (Sho= uld not create any errors) > + Attempt to remove the device's hardware address. (Should fai= l) I assume the reason this is checked again is making sure it still fails when the device mac is not the only one in the pool, but we should probably explain that. > + Determine the device's mac address pool size, and fill the p= ool with fake addresses. > + Attempt to add another fake mac address, overloading the add= ress pool. (Should not work) We say "should fail" everywhere else. Doesn't really change anything, but it would probably flow a little better to keep this consistent. > + """ > + testpmd =3D TestPmdShell(self.sut_node) > + testpmd.start() > + mac_address =3D self._sut_port_ingress.mac_address > + try: > + testpmd.set_mac_addr(0, "00:00:00:00:00:00") > + self.verify(False, "Invalid mac address added.") > + except InteractiveCommandExecutionError: > + pass > + try: > + testpmd.set_mac_addr(0, mac_address, False) > + self.verify(False, "Default mac address removed.") > + except InteractiveCommandExecutionError: > + pass > + # Should be no errors adding this twice > + testpmd.set_mac_addr(0, f"'1' + {mac_address[1:]}") > + testpmd.set_mac_addr(0, f"'1' + {mac_address[1:]}") > + # Double check to see if default mac address can be removed > + try: > + testpmd.set_mac_addr(0, mac_address, False) > + self.verify(False, "Default mac address removed.") > + except InteractiveCommandExecutionError: > + pass > + > + for i in range(testpmd.show_port_info(0).max_mac_addresses_num -= 1): > + number =3D str(hex(i)[2:].zfill(4)) > + fake_address =3D f'{number[:2] + ":" + number[2:] + mac_addr= ess[5:]}'.upper() It's not immediately obvious to me what this is doing, maybe a comment of an example about what this would look like for iteration 1 vs iteration 2 would help make it clear. > + testpmd.set_mac_addr(0, fake_address, verify=3DFalse) > + try: > + testpmd.set_mac_addr(0, f'{mac_address[:-1] + "1"}') What happens here if the last character in the MAC address already is 1? This would fail the test case right? > + self.verify(False, "Mac address limit exceeded.") > + except InteractiveCommandExecutionError: > + pass > + testpmd.close() > + > + def test_multicast_filter(self) -> None: > + """Assess basic multicast address filtering functionalities. > + > + Ensure that multicast filtering performs as intended when a give= n device is bound > + to the PMD, with and without dot1q vlan tagging. > + > + Test: > + Start TestPMD with promiscuous mode. > + Add a fake multicast address to the PMD's multicast address = pool. > + Send a packet with the fake multicast address to the PMD. (S= hould receive) > + Set vlan filtering on the PMD, and add vlan ID to the PMD. This test case seems like it is trying to test the multicast mac filtering, do we need to worry about VLAN filtering here as well, or just simply test when the VLAN tag is present? If possible it's probably better to avoid the testing of VLAN filtering since that is a different suite and I would assume that the functionality of filtering based on VLANs isn't directly related to filtering based on MAC addresses but I could be wrong. > + Send a packet with the fake multicast address and vlan ID to= the PMD. (Should receive) > + Send a packet with the fake multicast address and a differen= t vlan ID to the PMD. > + (Should not receive) > + Remove the vlan tag from the PMD, and turn vlan filtering of= f on the PMD. > + Send a packet with the fake multicast address and no vlan ta= g to the PMD. > + (Should receive) > + Remove the fake multicast address from the PMDs multicast ad= dress filter. > + Send a packet with the fake multicast address to the PMD. (S= hould not receive) > + """ > -- > 2.44.0 >