Netdev Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Cong Wang <xiyou.wangcong@gmail.com>
To: Jakub Kicinski <kuba@kernel.org>
Cc: David Miller <davem@davemloft.net>,
	Linux Kernel Network Developers <netdev@vger.kernel.org>,
	"Cong Wang ." <cong.wang@bytedance.com>,
	Peilin Ye <peilin.ye@bytedance.com>
Subject: Re: [PATCH net-next] Revert "netdevsim: Add multi-queue support"
Date: Tue, 3 Aug 2021 14:32:19 -0700	[thread overview]
Message-ID: <CAM_iQpV07aWSt5Jf-zSv6Qh4ydrJMYw54X3Seb8-eKGOpBYX7A@mail.gmail.com> (raw)
In-Reply-To: <20210803141839.79e99e23@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com>

On Tue, Aug 3, 2021 at 2:18 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Tue, 3 Aug 2021 10:11:13 -0700 Cong Wang wrote:
> > On Tue, Aug 3, 2021 at 5:39 AM Jakub Kicinski <kuba@kernel.org> wrote:
> > >
> > > This reverts commit d4861fc6be581561d6964700110a4dede54da6a6.
> > >
> > > netdevsim is for enabling upstream tests, two weeks in
> > > and there's no sign of upstream test using the "mutli-queue"
> > > option.
> >
> > Since when netdevsim is *only* for upstream tests?
>
> Since it was created.

Why it was created only for upstream? IOW, what's wrong with
using it only for non-upstream tests?

BTW, we also use dummy device for testing, it is not only for
upstream. It is extremely odd to single netdevsim out. I don't
see any special reason here.

>
> > Even if so, where is this documented? And why not just point it
> > out when reviewing it instead of silently waiting for weeks?
>
> I was AFK for the last two weeks.

How about documenting it in netdev-FAQ (or literally any doc)?
This would save everyone's time.

>
> > > We can add this option back when such test materializes.
> > > Right now it's dead code.
> >
> > It is clearly not dead. We internally used it for testing sch_mq,
> > this is clearly stated in the git log.
>
> Please contribute those tests upstream or keep any test harness
> they require where such test are, out of tree.

Peilin will add tc-testing for sch_mq which requires this netdevsim
feature.

>
> > How did you draw such a conclusion without talking to authors?
>
> There is no upstream test using this code, and I did CC you, didn't I?

There are downstream tests, which are mentioned in changelog.

I am pretty sure upstream tests only cover part of the whole networking
code, if you really want to apply the rule, a lot of code are already dead.
Once again, I don't see any reason why you only treat netdevsim differently.
;)

>
> > But this does remind me of using netdevsim for tc-testing.
>
> Please bring the code back as part of the series adding upstream tests.

Please remove all those not covered by upstream tests just to be fair??

Thank you!

  reply	other threads:[~2021-08-03 21:32 UTC|newest]

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-08-03 12:39 Jakub Kicinski
2021-08-03 17:11 ` Cong Wang
2021-08-03 21:18   ` Jakub Kicinski
2021-08-03 21:32     ` Cong Wang [this message]
2021-08-03 21:51       ` Jakub Kicinski
2021-08-03 22:04         ` Cong Wang
2021-08-04  7:14         ` Leon Romanovsky
2021-08-04 11:52           ` Jakub Kicinski
2021-08-04 12:49             ` Leon Romanovsky
2021-08-04 15:56               ` Jakub Kicinski
2021-08-04 17:25                 ` Leon Romanovsky
2021-08-03 22:16 ` [PATCH net-next] tc-testing: Add control-plane selftests for sch_mq Peilin Ye
2021-08-03 22:21   ` Cong Wang
2021-08-04 11:50   ` patchwork-bot+netdevbpf

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CAM_iQpV07aWSt5Jf-zSv6Qh4ydrJMYw54X3Seb8-eKGOpBYX7A@mail.gmail.com \
    --to=xiyou.wangcong@gmail.com \
    --cc=cong.wang@bytedance.com \
    --cc=davem@davemloft.net \
    --cc=kuba@kernel.org \
    --cc=netdev@vger.kernel.org \
    --cc=peilin.ye@bytedance.com \
    --subject='Re: [PATCH net-next] Revert "netdevsim: Add multi-queue support"' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).