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 A35FF45CD1; Wed, 13 Nov 2024 08:29:49 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 6C29940E0A; Wed, 13 Nov 2024 08:29:49 +0100 (CET) Received: from mail-pj1-f46.google.com (mail-pj1-f46.google.com [209.85.216.46]) by mails.dpdk.org (Postfix) with ESMTP id AA7BF400D6 for ; Wed, 13 Nov 2024 08:29:47 +0100 (CET) Received: by mail-pj1-f46.google.com with SMTP id 98e67ed59e1d1-2e2e88cb0bbso5175229a91.3 for ; Tue, 12 Nov 2024 23:29:47 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=iol.unh.edu; s=unh-iol; t=1731482986; x=1732087786; darn=dpdk.org; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=NK3rXODiSAPnyGZpPlyzy2oPezbXdEZZ003TPja1/Ks=; b=gkEHOt8AfrA9b6nhLqQjYpOH0eyS/+0nSOtxDkN09eE2gXHe1JxC6JPYR0Uvj1C9cb gPzQJUPErljRyBPodF7BOv/uNdwS+/VLh0+qRl1qYFohmnnAvOf970Ots3Ws1M6fQeBx nJ3SoZ9l0MXnwqzAsMFfZRbe8nl6rXu3jbDfk= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1731482986; x=1732087786; h=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=NK3rXODiSAPnyGZpPlyzy2oPezbXdEZZ003TPja1/Ks=; b=KzTXdp9DpbyH6Efr14GRVUTdM5ogYo6o/DOvHGegxgvXeZNrwvNr1lGnPuxJn6kF2/ XhojZFJ9vSWuPjTuzseY50BbcwLZQSElGbl0TSZL0RCKCfVtZ4lrQOjIhoTMW8g8nY6c P4FLijL/Xw3hf+80qBqSX99lrSi1PTTDolWWgLJZvQ0R6dJuTckHN/zxSyfd+hslyZDE ekYbn+8pnXBDs3ffSFEj5hnh7L1Ik+cMgG9RsxzkLlyKrsvrxCu8uzfG7I51LfT+liYc iQpbPf9MxhmXZkjE5XPzrcMtWY8tXC725B80KTstLJV/xe6OPbqViWh6RUzKZ3n8s0oe WMrw== X-Forwarded-Encrypted: i=1; AJvYcCUKKeXPhHz6bc5zBlJSckJP0xxqx5xNSZR26WKd0T00Nd59zrOTZI4eqpgyZa4MMVc5xiQ=@dpdk.org X-Gm-Message-State: AOJu0YynXEzrwGRTOFnS31CSkvDMWZ0bUc2JC79NpnYI8Rp7X4MSNH29 sQQycxkX5+TQzjZpxhxKvV0hxkYKHISc/9BSaeH5dNVQKWw3T8iQPvor6oqppqIxxrdnz8ppzpT OVbLxmSf+5d9UT1TjH6CJFGoGTJDoxOVQyiRMDg== X-Google-Smtp-Source: AGHT+IFb1u7YrQQrRyL6LcIb+xxexnrjl7MBJyk/fkFY2rZOND3h5qENLjtNHVS1JhRpuGegZ3HzMWZwiVqilQEMz+A= X-Received: by 2002:a17:90b:540b:b0:2e1:682b:361a with SMTP id 98e67ed59e1d1-2e9b177fce7mr25595152a91.28.1731482986548; Tue, 12 Nov 2024 23:29:46 -0800 (PST) MIME-Version: 1.0 References: <20241030170808.29452-1-npratte@iol.unh.edu> In-Reply-To: From: Patrick Robb Date: Wed, 13 Nov 2024 02:27:52 -0500 Message-ID: Subject: Re: [PATCH v1] dts: add time delay to async sniffer callback function To: Paul Szczepanek Cc: Nicholas Pratte , yoan.picchi@foss.arm.com, dmarx@iol.unh.edu, Honnappa.Nagarahalli@arm.com, luca.vizzarro@arm.com, nd@arm.com, dev@dpdk.org Content-Type: multipart/alternative; boundary="000000000000592bdc0626c64c89" 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 --000000000000592bdc0626c64c89 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Wed, Nov 6, 2024 at 3:17=E2=80=AFPM Paul Szczepanek wrote: > > On 30/10/2024 17:08, Nicholas Pratte wrote: > > There exists a bug within i40e NICs in which the async sniffer does not > > catch send packets as a result of the callback function sending packets > > too quickly before the NICs are ready to start capturing. > > > > There could be a multitude of reasons why this happens on these NICs, b= ut > > for the time being, inserting a one second delay in the callback functi= on > > will suffice. > > I can confirm the issue exists but we should explore a more definitive > solution than adding a wait. Ideally instead of relying on the callback > to send packets we should verify readines elsewhere in our sniffer and > send packets when ready in our framework and not as part of the scapy > sniffer constructor. > >From looking at the documentation, it is the case that the standard way of verifying readiness for the asyncsniffer is via the started_callback arg in the asyncsniffer constructor. You can see some similar discussion here: https://github.com/secdev/scapy/issues/3208 So, if this is standard, it is probably best to remain within this framework. I have been messing with this series tonight and although I still can't tell why started_callback isn't calling on true sniffer readiness, I think Nick's time.sleep calls are okay. I will say, the modification of duration in this series is odd to me. It looks like the _shell_start_and_stop_sniffing function arg has no default, and no value is passed in in the call coming from send_packets_and_capture. My preference would be to provide a default to the duration arg (say, 1) and remove the arbitrary "duration + 1" in this series. I also believe the comments about i40e should be removed. We understand that this series is adding a delay to support sniffer readiness, but we don't know why this behavior was originally seen on an i40e NIC, and whether it's isolated to that driver. Perhaps there would be a way to loop polling of scapy for sniffer readiness, but I don't see how this would be better or different than the asyncsniffer callback arg (which is essentially the same according to docs). So, in my view the best thing to do is for me to fix up the commit per my comments (and any others anyone has) and apply this. --000000000000592bdc0626c64c89 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable


=
On Wed, Nov 6, 2024 at 3:17=E2=80=AFP= M Paul Szczepanek <paul.szcze= panek@arm.com> wrote:

On 30/10/2024 17:08, Nicholas Pratte wrote:
> There exists a bug within i40e NICs in which the async sniffer does no= t
> catch send packets as a result of the callback function sending packet= s
> too quickly before the NICs are ready to start capturing.
>
> There could be a multitude of reasons why this happens on these NICs, = but
> for the time being, inserting a one second delay in the callback funct= ion
> will suffice.

I can confirm the issue exists but we should explore a more definitive
solution than adding a wait. Ideally instead of relying on the callback
to send packets we should verify readines elsewhere in our sniffer and
send packets when ready in our framework and not as part of the scapy
sniffer constructor.

From looking at th= e documentation, it is the case that the standard way of verifying readines= s for the asyncsniffer is via the started_callback arg in the asyncsniffer= =C2=A0constructor. You can see some similar discussion here:=C2=A0https://github.com/secdev/= scapy/issues/3208

=C2=A0So, if this is standar= d, it is probably best to remain within this framework. I have been messing= with this series tonight and although I still can't tell why started_c= allback isn't calling on true sniffer readiness, I think Nick's tim= e.sleep calls are okay.

I will say, the modificati= on of duration in this series is odd to me. It looks like the=C2=A0_shell_s= tart_and_stop_sniffing function arg has no default, and no value is passed = in in the call coming from send_packets_and_capture. My preference would be= to provide a default to the duration arg (say, 1) and remove the arbitrary= "duration=C2=A0+ 1" in this series.

I a= lso believe the comments about i40e should be removed. We understand that t= his series is adding a delay to support sniffer readiness, but we don't= know why this behavior was originally seen on an i40e NIC, and whether it&= #39;s isolated to that driver.

Perhaps there would= be a way to loop polling of scapy for sniffer readiness, but I don't s= ee how this would be better or different than the asyncsniffer=C2=A0callbac= k arg (which is essentially the same according to docs). So, in my view the= best thing to do is for me to fix up the commit per my comments (and any o= thers anyone has) and apply this.


=
--000000000000592bdc0626c64c89--