Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Mon, 31 Oct 2022 21:17:08 +0100
From:   Andrew Lunn <andrew@lunn.ch>
To:     Shenwei Wang <shenwei.wang@nxp.com>
Cc:     "David S. Miller" <davem@davemloft.net>,
        Eric Dumazet <edumazet@google.com>,
        Jakub Kicinski <kuba@kernel.org>,
        Paolo Abeni <pabeni@redhat.com>,
        Alexei Starovoitov <ast@kernel.org>,
        Daniel Borkmann <daniel@iogearbox.net>,
        Jesper Dangaard Brouer <hawk@kernel.org>,
        John Fastabend <john.fastabend@gmail.com>,
        "netdev@vger.kernel.org" <netdev@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "imx@lists.linux.dev" <imx@lists.linux.dev>,
        kernel test robot <lkp@intel.com>
Subject: Re: [EXT] Re: [PATCH v2 1/1] net: fec: add initial XDP support
Message-ID: <Y2AtRL8l1/kZrwx7@lunn.ch>
References: <20221031162200.1997788-1-shenwei.wang@nxp.com>
 <Y2ABb+G+ykcUd413@lunn.ch>
 <PAXPR04MB918581FFA58483608B3A7DCA89379@PAXPR04MB9185.eurprd04.prod.outlook.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <PAXPR04MB918581FFA58483608B3A7DCA89379@PAXPR04MB9185.eurprd04.prod.outlook.com>
Precedence: bulk

> > > +                     cbd_base = rxq->bd.base;
> > > +                     if (bpf->prog)
> > > +                             rxq->bd.ring_size = XDP_RX_RING_SIZE;
> > > +                     else
> > > +                             rxq->bd.ring_size = RX_RING_SIZE;
> > > +                     size = dsize * rxq->bd.ring_size;
> > > +                     cbd_base = (struct bufdesc *)(((void *)cbd_base) + size);
> > > +                     rxq->bd.last = (struct bufdesc *)(((void
> > > + *)cbd_base) - dsize);
> > 
> > This does not look safe. netif_tx_disable(dev) will stop new transmissions, but
> > the hardware can be busy receiving, DMAing frames, using the descriptors, etc.
> > Modifying rxq->bd.last in particular seems risky. I think you need to disable the
> > receiver, wait for it to indicate it really has stopped, and only then make these
> > modifications.
> > 
> 
> Sounds reasonable. How about moving the codes of updating ring size to the place
> right after the enet reset inside the fec_restart? This should clear those risky corner
> cases.

That sounds reasonable. But please add some comments. The driver has
RX_RING_SIZE elements allocated, but you are only using a subset. This
needs to be clear for when somebody implements the ethtool --rings
option.

And i still think it would be good to implement that code. As your
numbers show, the ring size does affect performance, and it is hard to
know if your hard coded XDP_RX_RING_SIZE is the right value, depending
on what the eBPF program is doing. If the ethtool option was provided,
it allows users to benchmark different ring sizes for their workload.

   Andrew