Message-ID: <20200629180035.huq42fif7wktfbja@kafai-mbp.dhcp.thefacebook.com>
Date:   Mon, 29 Jun 2020 11:00:35 -0700
From:   Martin KaFai Lau <kafai@...com>
To:     Andrii Nakryiko <andrii.nakryiko@...il.com>
CC:     bpf <bpf@...r.kernel.org>, Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Kernel Team <kernel-team@...com>,
        Lawrence Brakmo <brakmo@...com>,
        Neal Cardwell <ncardwell@...gle.com>,
        Networking <netdev@...r.kernel.org>,
        Yuchung Cheng <ycheng@...gle.com>
Subject: Re: [PATCH bpf-next 07/10] bpf: selftests: Restore netns after each
 test

On Sat, Jun 27, 2020 at 01:31:42PM -0700, Andrii Nakryiko wrote:
> On Fri, Jun 26, 2020 at 5:23 PM Martin KaFai Lau <kafai@...com> wrote:
> >
> > On Fri, Jun 26, 2020 at 03:45:04PM -0700, Andrii Nakryiko wrote:
> > > On Fri, Jun 26, 2020 at 10:56 AM Martin KaFai Lau <kafai@...com> wrote:
> > > >
> > > > It is common for networking tests creating its netns and making its own
> > > > setting under this new netns (e.g. changing tcp sysctl).  If the test
> > > > forgot to restore to the original netns, it would affect the
> > > > result of other tests.
> > > >
> > > > This patch saves the original netns at the beginning and then restores it
> > > > after every test.  Since the restore "setns()" is not expensive, it does it
> > > > on all tests without tracking if a test has created a new netns or not.
> > > >
> > > > Signed-off-by: Martin KaFai Lau <kafai@...com>
> > > > ---
> > > >  tools/testing/selftests/bpf/test_progs.c | 21 +++++++++++++++++++++
> > > >  tools/testing/selftests/bpf/test_progs.h |  2 ++
> > > >  2 files changed, 23 insertions(+)
> > > >
> > > > diff --git a/tools/testing/selftests/bpf/test_progs.c b/tools/testing/selftests/bpf/test_progs.c
> > > > index 54fa5fa688ce..b521ce366381 100644
> > > > --- a/tools/testing/selftests/bpf/test_progs.c
> > > > +++ b/tools/testing/selftests/bpf/test_progs.c
> > > > @@ -121,6 +121,24 @@ static void reset_affinity() {
> > > >         }
> > > >  }
> > > >
> > > > +static void save_netns(void)
> > > > +{
> > > > +       env.saved_netns_fd = open("/proc/self/ns/net", O_RDONLY);
> > > > +       if (env.saved_netns_fd == -1) {
> > > > +               perror("open(/proc/self/ns/net)");
> > > > +               exit(-1);
> > > > +       }
> > > > +}
> > > > +
> > > > +static void restore_netns(void)
> > > > +{
> > > > +       if (setns(env.saved_netns_fd, CLONE_NEWNET) == -1) {
> > > > +               stdio_restore();
> > > > +               perror("setns(CLONE_NEWNS)");
> > > > +               exit(-1);
> > > > +       }
> > > > +}
> > > > +
> > > >  void test__end_subtest()
> > > >  {
> > > >         struct prog_test_def *test = env.test;
> > > > @@ -643,6 +661,7 @@ int main(int argc, char **argv)
> > > >                 return -1;
> > > >         }
> > > >
> > > > +       save_netns();
> > >
> > > you should probably do this also after each sub-test in test__end_subtest()?
> > You mean restore_netns()?
> 
> oops, yeah :)
> 
> >
> > It is a tough call.
> > Some tests may only want to create a netns at the beginning for all the subtests
> > to use (e.g. sk_assign.c).  restore_netns() after each subtest may catch
> > tester in surprise that the netns is not in its full control while its
> > own test is running.
> 
> Wouldn't it be better to update such self-tests to setns on each
> sub-test properly? It should be a simple code re-use exercise, unless
> I'm missing some other implications of having to do it before each
> sub-test?
It should be simple, I think.  Haven't looked into details of each test.
However, I won't count re-running the same piece of code in a for-loop
as a re-use exercise ;)

In my vm, a quick try in forcing sk_assign.c to reconfigure netns in each
subtest in the for loop increased the runtime from 1s to 8s.
I guess it is not a big deal for test_progs.

> 
> The idea behind sub-test is (at least it was so far) that it's
> independent from other sub-tests and tests, and it's only co-located
> with other sub-tests for the purpose of code reuse and logical
> grouping. Which is why we reset CPU affinity, for instance.
> 
> >
> > I think an individual test should have managed the netns properly within its
> > subtests already if it wants a correct test result.  It can unshare at the
> > beginning of each subtest to get a clean state (e.g. in patch 8).
> > test_progs.c only ensures a config made by an earlier test does
> > not affect the following tests.
> 
> It's true that it gives more flexibility for test setup, but if we go
> that way, we should do it consistently for CPU affinity resetting and
> whatever else we do per-subtest. I wonder what your answers would be
> for the above questions. We can go either way, just let's be
> consistent.
Right, I also don't feel strongly about which way to go for netns.
I noticed reset_affinity().  cgroup cleanup is also per test though.
I think netns is more close to cgroup in terms of bpf prog is running under it,
so this patch picked the current way.

If it is decided to stay with reset_affinity's way,  I can make netns change
to other tests (there are two if i grep properly).

It seems there is no existing subtest requires to reset_affinity.

Powered by blists - more mailing lists