Subject: Re: [PATCH net-next v4 9/9] selftests: ncdevmem: Implement devmem TCP TX

From: Stanislav Fomichev <[email-masked]>
Date: Fri, 21 Feb 2025 16:20:49 -0800

On 02/21, Mina Almasry wrote:
> Hi Stan,
> 
> Thank you very much for testing. I was wondering/worried that there
> will be some churn in getting the test working on both our setups.
> It's not unheard of I think because your ncdevmem changes had to go
> through a couple of iterations to work for our slightly different
> setups, but do bear with me. Thanks!
> 
> On Thu, Feb 20, 2025 at 12:01 PM Stanislav Fomichev
> <[email-masked]> wrote:
> > > @@ -25,18 +25,36 @@ def check_rx(cfg) -> None:
> > >      require_devmem(cfg)
> > >
> > >      port = rand_port()
> > > -    listen_cmd = f"./ncdevmem -l -f {cfg.ifname} -s {cfg.v6} -p {port}"
> > > +    listen_cmd = f"{cfg.bin_local} -l -f {cfg.ifname} -s {cfg.v6} -p {port}"
> > >
> > >      with bkg(listen_cmd) as socat:
> > >          wait_port_listen(port)
> > > -        cmd(f"echo -e \"hello\\nworld\"| socat -u - TCP6:[{cfg.v6}]:{port}", host=cfg.remote, shell=True)
> > > +        cmd(f"echo -e \"hello\\nworld\"| socat -u - TCP6:{cfg.v6}:{port},bind={cfg.remote_v6}:{port}", host=cfg.remote, shell=True)
> >
> > IPv6 address need to be wrapped into [], so has to be at least:
> >         socat -u - TCP6:[{cfg.v6}]:{port},bind=[{cfg.remote_v6}]:{port}
> >
> 
> Yeah, I will need to propagate the ncdevmem ipv4 support to devmem.py
> in the future, but unnecessary for this series. Will do.
> 
> > But not sure why we care here about bind address here, let the kernel
> > figure out the routing.
> >
> 
> I will need to add this in the future to support my 5-tuple flow
> steering setup in the future, but it is indeed unnecessary for this
> series. Additionally the bind in the check_tx test is unnecessary,
> removed there as well. Lets see if it works for you.

Hmm, true that it's not needed in check_tx as well. Let's drop from
check_tx and introduce when you need it? (but up to you really,
was just wondering why change rx side..)