| Message ID | 20230801061932.10335-1-liangchen.linux@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [RFC,net-next,v2,1/2] net: veth: Page pool creation error handling for existing pools only | expand |
On Tue, Aug 01, 2023 at 02:19:31PM +0800, Liang Chen wrote: > The failure handling procedure destroys page pools for all queues, > including those that haven't had their page pool created yet. this patch > introduces necessary adjustments to prevent potential risks and > inconsistency with the error handling behavior. > > Signed-off-by: Liang Chen <liangchen.linux@gmail.com> I wonder if this should this have a fixes tag and be targeted at 'net' as a bugfix?
On 01/08/2023 08.19, Liang Chen wrote: > The failure handling procedure destroys page pools for all queues, > including those that haven't had their page pool created yet. this patch > introduces necessary adjustments to prevent potential risks and > inconsistency with the error handling behavior. > > Signed-off-by: Liang Chen <liangchen.linux@gmail.com> > --- > drivers/net/veth.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/net/veth.c b/drivers/net/veth.c > index 614f3e3efab0..509e901da41d 100644 > --- a/drivers/net/veth.c > +++ b/drivers/net/veth.c > @@ -1081,8 +1081,9 @@ static int __veth_napi_enable_range(struct net_device *dev, int start, int end) > err_xdp_ring: > for (i--; i >= start; i--) > ptr_ring_cleanup(&priv->rq[i].xdp_ring, veth_ptr_free); > + i = end; > err_page_pool: > - for (i = start; i < end; i++) { > + for (i--; i >= start; i--) { I'm not a fan of this coding style, that iterates backwards, but I can see you just inherited the existing style in this function. > page_pool_destroy(priv->rq[i].page_pool); > priv->rq[i].page_pool = NULL; > } The page_pool_destroy() call handles(exits) if called with NULL. So, I don't think this incorrect walking all (start to end) can trigger an actual bug. Anyhow, I do think this is more correct, so you can append my ACK for the real submission. Acked-by: Jesper Dangaard Brouer <hawk@kernel.org>
On Tue, Aug 1, 2023 at 8:18 PM Simon Horman <horms@kernel.org> wrote: > > On Tue, Aug 01, 2023 at 02:19:31PM +0800, Liang Chen wrote: > > The failure handling procedure destroys page pools for all queues, > > including those that haven't had their page pool created yet. this patch > > introduces necessary adjustments to prevent potential risks and > > inconsistency with the error handling behavior. > > > > Signed-off-by: Liang Chen <liangchen.linux@gmail.com> > > I wonder if this should this have a fixes tag and be targeted at 'net' as a > bugfix? It doesn't trigger a real bug at the moment as page_pool_destroy handles NULL argument. So may not be suitable for a fix tag. Thanks, Liang
On Wed, Aug 2, 2023 at 4:56 PM Jesper Dangaard Brouer <hawk@kernel.org> wrote: > > > > On 01/08/2023 08.19, Liang Chen wrote: > > The failure handling procedure destroys page pools for all queues, > > including those that haven't had their page pool created yet. this patch > > introduces necessary adjustments to prevent potential risks and > > inconsistency with the error handling behavior. > > > > Signed-off-by: Liang Chen <liangchen.linux@gmail.com> > > --- > > drivers/net/veth.c | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/net/veth.c b/drivers/net/veth.c > > index 614f3e3efab0..509e901da41d 100644 > > --- a/drivers/net/veth.c > > +++ b/drivers/net/veth.c > > @@ -1081,8 +1081,9 @@ static int __veth_napi_enable_range(struct net_device *dev, int start, int end) > > err_xdp_ring: > > for (i--; i >= start; i--) > > ptr_ring_cleanup(&priv->rq[i].xdp_ring, veth_ptr_free); > > + i = end; > > err_page_pool: > > - for (i = start; i < end; i++) { > > + for (i--; i >= start; i--) { > > I'm not a fan of this coding style, that iterates backwards, but I can > see you just inherited the existing style in this function. > > > page_pool_destroy(priv->rq[i].page_pool); > > priv->rq[i].page_pool = NULL; > > } > > The page_pool_destroy() call handles(exits) if called with NULL. > So, I don't think this incorrect walking all (start to end) can trigger > an actual bug. > > Anyhow, I do think this is more correct, so you can append my ACK for > the real submission. > > Acked-by: Jesper Dangaard Brouer <hawk@kernel.org> > Thanks! I will separate this patch out and make a real submission, since it's a small fix and not really coupled with the optimization patch which still needs some further work after receiving feedback from Yunsheng. Thanks, Liang
On 11/08/2023 14.02, Liang Chen wrote: > On Wed, Aug 2, 2023 at 4:56 PM Jesper Dangaard Brouer <hawk@kernel.org> wrote: >> [...] >>> page_pool_destroy(priv->rq[i].page_pool); >>> priv->rq[i].page_pool = NULL; >>> } >> >> The page_pool_destroy() call handles(exits) if called with NULL. >> So, I don't think this incorrect walking all (start to end) can trigger >> an actual bug. >> >> Anyhow, I do think this is more correct, so you can append my ACK for >> the real submission. >> >> Acked-by: Jesper Dangaard Brouer <hawk@kernel.org> >> > > Thanks! I will separate this patch out and make a real submission, > since it's a small fix and not really coupled with the optimization > patch which still needs some further work after receiving feedback > from Yunsheng. Sure, send it as a fix commit. Given it is not super critical i think it is okay to send for net-next, to avoid merge issues/conflicts with your 2/2 optimization patch. And for good order we should add a Fixes tag, but IMHO net-next is still okay, given I don't think this can trigger a bug. That said, I do want to encourage you to work on 2/2 optimization patch. I think this is a very important optimization and actually a fix for then we introduced page_pool to veth. Well spotted! :-) In 2/2 I keep getting confused by use of kfree_skb_partial() and if the right conditions are meet. --Jesper
On Sat, Aug 12, 2023 at 12:35 AM Jesper Dangaard Brouer <jbrouer@redhat.com> wrote: > > > > On 11/08/2023 14.02, Liang Chen wrote: > > On Wed, Aug 2, 2023 at 4:56 PM Jesper Dangaard Brouer <hawk@kernel.org> wrote: > >> > [...] > >>> page_pool_destroy(priv->rq[i].page_pool); > >>> priv->rq[i].page_pool = NULL; > >>> } > >> > >> The page_pool_destroy() call handles(exits) if called with NULL. > >> So, I don't think this incorrect walking all (start to end) can trigger > >> an actual bug. > >> > >> Anyhow, I do think this is more correct, so you can append my ACK for > >> the real submission. > >> > >> Acked-by: Jesper Dangaard Brouer <hawk@kernel.org> > >> > > > > Thanks! I will separate this patch out and make a real submission, > > since it's a small fix and not really coupled with the optimization > > patch which still needs some further work after receiving feedback > > from Yunsheng. > > Sure, send it as a fix commit. Given it is not super critical i think > it is okay to send for net-next, to avoid merge issues/conflicts with > your 2/2 optimization patch. And for good order we should add a Fixes > tag, but IMHO net-next is still okay, given I don't think this can > trigger a bug. > > That said, I do want to encourage you to work on 2/2 optimization patch. > I think this is a very important optimization and actually a fix for > then we introduced page_pool to veth. Well spotted! :-) > > In 2/2 I keep getting confused by use of kfree_skb_partial() and if the > right conditions are meet. > > --Jesper > Thank you for the encouragement! The work for the optimization patch is going on, and we will submit another version which hopefully will give a further performance improvement.
diff --git a/drivers/net/veth.c b/drivers/net/veth.c index 614f3e3efab0..509e901da41d 100644 --- a/drivers/net/veth.c +++ b/drivers/net/veth.c @@ -1081,8 +1081,9 @@ static int __veth_napi_enable_range(struct net_device *dev, int start, int end) err_xdp_ring: for (i--; i >= start; i--) ptr_ring_cleanup(&priv->rq[i].xdp_ring, veth_ptr_free); + i = end; err_page_pool: - for (i = start; i < end; i++) { + for (i--; i >= start; i--) { page_pool_destroy(priv->rq[i].page_pool); priv->rq[i].page_pool = NULL; }
The failure handling procedure destroys page pools for all queues, including those that haven't had their page pool created yet. this patch introduces necessary adjustments to prevent potential risks and inconsistency with the error handling behavior. Signed-off-by: Liang Chen <liangchen.linux@gmail.com> --- drivers/net/veth.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)