All of lore.kernel.org
 help / color / mirror / Atom feed
From: Paolo Abeni <pabeni@redhat.com>
To: "Toke Høiland-Jørgensen" <toke@redhat.com>, netdev@vger.kernel.org
Cc: "David S. Miller" <davem@davemloft.net>,
	Jakub Kicinski <kuba@kernel.org>
Subject: Re: [RFC PATCH 1/3] veth: implement support for set_channel ethtool op
Date: Fri, 09 Jul 2021 16:38:02 +0200	[thread overview]
Message-ID: <9c451d25fb36bc82e602bb93e384b262be743fbf.camel@redhat.com> (raw)
In-Reply-To: <1c3d691c01121e4110f23d5947b2809d5cce056b.camel@redhat.com>

On Fri, 2021-07-09 at 12:49 +0200, Paolo Abeni wrote:
> On Fri, 2021-07-09 at 12:15 +0200, Toke Høiland-Jørgensen wrote:
> > > +	if (netif_running(dev))
> > > +		veth_close(dev);
> > > +
> > > +	priv->num_tx_queues = ch->tx_count;
> > > +	priv->num_rx_queues = ch->rx_count;
> > 
> > Why can't just you use netif_set_real_num_*_queues() here directly (and
> > get rid of the priv members as above)?
> 
> Uhm... I haven't thought about it. Let me try ;)

Here there is a possible problem: if the latter
netif_set_real_num_*_queues() fails, we should not change the current
configuration, so we should revert the
first netif_set_real_num_*_queues() change.

Even that additional revert operation could fail. If/when that happen
set_channel() will leave the device in a different state from both the
old one and the new one, possibly with an XDP-incompatible number of
queues.

Keeping the  netif_set_real_num_*_queues() calls in veth_open() avoid
the above issue: if the queue creation is problematic, the device will
stay down.

I think the additional fields are worthy, WDYT? 

Cheers,
Paolo




  parent reply	other threads:[~2021-07-09 14:38 UTC|newest]

Thread overview: 23+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-07-09  9:39 [RFC PATCH 0/3] veth: more flexible channels number configuration Paolo Abeni
2021-07-09  9:39 ` [RFC PATCH 1/3] veth: implement support for set_channel ethtool op Paolo Abeni
2021-07-09 10:15   ` Toke Høiland-Jørgensen
2021-07-09 10:49     ` Paolo Abeni
2021-07-09 11:36       ` Toke Høiland-Jørgensen
2021-07-09 14:38       ` Paolo Abeni [this message]
2021-07-09 15:23         ` Toke Høiland-Jørgensen
2021-07-09 16:35           ` Paolo Abeni
2021-07-09 19:56             ` Jakub Kicinski
2021-07-09 19:54   ` Jakub Kicinski
2021-07-12  1:44     ` David Ahern
2021-07-12 10:45       ` Paolo Abeni
2021-07-12 15:23         ` Jakub Kicinski
2021-07-10  8:43   ` kernel test robot
2021-07-09  9:39 ` [RFC PATCH 2/3] veth: make queues nr configurable via kernel module params Paolo Abeni
2021-07-09 10:24   ` Toke Høiland-Jørgensen
2021-07-09 15:33     ` Paolo Abeni
2021-07-09 16:12       ` Toke Høiland-Jørgensen
2021-07-09 14:25   ` kernel test robot
2021-07-09 16:59   ` kernel test robot
2021-07-10 17:52   ` kernel test robot
2021-07-10 17:52   ` [RFC PATCH] veth: veth_get_num_tx_queues() can be static kernel test robot
2021-07-09  9:39 ` [RFC PATCH 3/3] selftests: net: veth: add tests for set_channel Paolo Abeni

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=9c451d25fb36bc82e602bb93e384b262be743fbf.camel@redhat.com \
    --to=pabeni@redhat.com \
    --cc=davem@davemloft.net \
    --cc=kuba@kernel.org \
    --cc=netdev@vger.kernel.org \
    --cc=toke@redhat.com \
    /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
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.