| Message ID | 20231116022041.51959-1-lirongqing@baidu.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next] net/smc: avoid atomic_set and smp_wmb in the tx path when possible | expand |
On Thu, Nov 16, 2023 at 10:20:41AM +0800, Li RongQing wrote: >there is rare possibility that conn->tx_pushing is not 1, since >tx_pushing is just checked with 1, so move the setting tx_pushing >to 1 after atomic_dec_and_test() return false, to avoid atomic_set >and smp_wmb in tx path > >Signed-off-by: Li RongQing <lirongqing@baidu.com> Reviewed-by: Dust Li <dust.li@linux.alibaba.com> >--- > net/smc/smc_tx.c | 7 ++++--- > 1 file changed, 4 insertions(+), 3 deletions(-) > >diff --git a/net/smc/smc_tx.c b/net/smc/smc_tx.c >index 3b0ff3b..72dbdee 100644 >--- a/net/smc/smc_tx.c >+++ b/net/smc/smc_tx.c >@@ -667,8 +667,6 @@ int smc_tx_sndbuf_nonempty(struct smc_connection *conn) > return 0; > > again: >- atomic_set(&conn->tx_pushing, 1); >- smp_wmb(); /* Make sure tx_pushing is 1 before real send */ > rc = __smc_tx_sndbuf_nonempty(conn); > > /* We need to check whether someone else have added some data into >@@ -677,8 +675,11 @@ int smc_tx_sndbuf_nonempty(struct smc_connection *conn) > * If so, we need to push again to prevent those data hang in the send > * queue. > */ >- if (unlikely(!atomic_dec_and_test(&conn->tx_pushing))) >+ if (unlikely(!atomic_dec_and_test(&conn->tx_pushing))) { >+ atomic_set(&conn->tx_pushing, 1); >+ smp_wmb(); /* Make sure tx_pushing is 1 before real send */ nit: it would be better if we change the comments to "send again". Thanks > goto again; >+ } > > return rc; > } >-- >2.9.4
> -----Original Message----- > From: Dust Li <dust.li@linux.alibaba.com> > Sent: Thursday, November 16, 2023 2:18 PM > To: Li,Rongqing <lirongqing@baidu.com>; wenjia@linux.ibm.co; > netdev@vger.kernel.org; linux-s390@vger.kernel.org > Subject: Re: [PATCH][net-next] net/smc: avoid atomic_set and smp_wmb in the > tx path when possible > > On Thu, Nov 16, 2023 at 10:20:41AM +0800, Li RongQing wrote: > >there is rare possibility that conn->tx_pushing is not 1, since > >tx_pushing is just checked with 1, so move the setting tx_pushing to 1 > >after atomic_dec_and_test() return false, to avoid atomic_set and > >smp_wmb in tx path > > > >Signed-off-by: Li RongQing <lirongqing@baidu.com> > Reviewed-by: Dust Li <dust.li@linux.alibaba.com> > > >--- > > net/smc/smc_tx.c | 7 ++++--- > > 1 file changed, 4 insertions(+), 3 deletions(-) > > > >diff --git a/net/smc/smc_tx.c b/net/smc/smc_tx.c index 3b0ff3b..72dbdee > >100644 > >--- a/net/smc/smc_tx.c > >+++ b/net/smc/smc_tx.c > >@@ -667,8 +667,6 @@ int smc_tx_sndbuf_nonempty(struct smc_connection > *conn) > > return 0; > > > > again: > >- atomic_set(&conn->tx_pushing, 1); > >- smp_wmb(); /* Make sure tx_pushing is 1 before real send */ > > rc = __smc_tx_sndbuf_nonempty(conn); > > > > /* We need to check whether someone else have added some data into > @@ > >-677,8 +675,11 @@ int smc_tx_sndbuf_nonempty(struct smc_connection > *conn) > > * If so, we need to push again to prevent those data hang in the send > > * queue. > > */ > >- if (unlikely(!atomic_dec_and_test(&conn->tx_pushing))) > >+ if (unlikely(!atomic_dec_and_test(&conn->tx_pushing))) { > >+ atomic_set(&conn->tx_pushing, 1); > >+ smp_wmb(); /* Make sure tx_pushing is 1 before real send */ > nit: it would be better if we change the comments to "send again". > Ok, I will fix it, thanks -Li > Thanks > > goto again; > >+ } > > > > return rc; > > } > >-- > >2.9.4
On 2023/11/16 10:20, Li RongQing wrote: > there is rare possibility that conn->tx_pushing is not 1, since There > tx_pushing is just checked with 1, so move the setting tx_pushing > to 1 after atomic_dec_and_test() return false, to avoid atomic_set > and smp_wmb in tx path . > Some nits: 1. It is normally using [PATCH net-next] rather than [PATCH][net-next] in subject. And new version should better be marked. such as: # git format-patch --subject-prefix="PATCH net-next" -v 3 And CC all relevant people listed by: # ./scripts/get_maintainer.pl <your patch> 2. Few improvements in the commit body. Thanks, Wen Gu > Signed-off-by: Li RongQing <lirongqing@baidu.com> > --- > net/smc/smc_tx.c | 7 ++++--- > 1 file changed, 4 insertions(+), 3 deletions(-) > > diff --git a/net/smc/smc_tx.c b/net/smc/smc_tx.c > index 3b0ff3b..72dbdee 100644 > --- a/net/smc/smc_tx.c > +++ b/net/smc/smc_tx.c > @@ -667,8 +667,6 @@ int smc_tx_sndbuf_nonempty(struct smc_connection *conn) > return 0; > > again: > - atomic_set(&conn->tx_pushing, 1); > - smp_wmb(); /* Make sure tx_pushing is 1 before real send */ > rc = __smc_tx_sndbuf_nonempty(conn); > > /* We need to check whether someone else have added some data into > @@ -677,8 +675,11 @@ int smc_tx_sndbuf_nonempty(struct smc_connection *conn) > * If so, we need to push again to prevent those data hang in the send > * queue. > */ > - if (unlikely(!atomic_dec_and_test(&conn->tx_pushing))) > + if (unlikely(!atomic_dec_and_test(&conn->tx_pushing))) { > + atomic_set(&conn->tx_pushing, 1); > + smp_wmb(); /* Make sure tx_pushing is 1 before real send */ > goto again; > + } > > return rc; > }
> -----Original Message----- > From: Wen Gu <guwen@linux.alibaba.com> > Sent: Thursday, November 16, 2023 5:28 PM > To: Li,Rongqing <lirongqing@baidu.com>; wenjia@linux.ibm.co; > netdev@vger.kernel.org; linux-s390@vger.kernel.org; dust.li@linux.alibaba.com > Subject: Re: [PATCH][net-next] net/smc: avoid atomic_set and smp_wmb in the > tx path when possible > > > > On 2023/11/16 10:20, Li RongQing wrote: > > there is rare possibility that conn->tx_pushing is not 1, since > There > > tx_pushing is just checked with 1, so move the setting tx_pushing to 1 > > after atomic_dec_and_test() return false, to avoid atomic_set and > > smp_wmb in tx path > . > > > > Some nits: > > 1. It is normally using [PATCH net-next] rather than [PATCH][net-next] > in subject. And new version should better be marked. such as: > > # git format-patch --subject-prefix="PATCH net-next" -v 3 > > And CC all relevant people listed by: > > # ./scripts/get_maintainer.pl <your patch> > > 2. Few improvements in the commit body. > > Ok, thanks -Li
diff --git a/net/smc/smc_tx.c b/net/smc/smc_tx.c index 3b0ff3b..72dbdee 100644 --- a/net/smc/smc_tx.c +++ b/net/smc/smc_tx.c @@ -667,8 +667,6 @@ int smc_tx_sndbuf_nonempty(struct smc_connection *conn) return 0; again: - atomic_set(&conn->tx_pushing, 1); - smp_wmb(); /* Make sure tx_pushing is 1 before real send */ rc = __smc_tx_sndbuf_nonempty(conn); /* We need to check whether someone else have added some data into @@ -677,8 +675,11 @@ int smc_tx_sndbuf_nonempty(struct smc_connection *conn) * If so, we need to push again to prevent those data hang in the send * queue. */ - if (unlikely(!atomic_dec_and_test(&conn->tx_pushing))) + if (unlikely(!atomic_dec_and_test(&conn->tx_pushing))) { + atomic_set(&conn->tx_pushing, 1); + smp_wmb(); /* Make sure tx_pushing is 1 before real send */ goto again; + } return rc; }
there is rare possibility that conn->tx_pushing is not 1, since tx_pushing is just checked with 1, so move the setting tx_pushing to 1 after atomic_dec_and_test() return false, to avoid atomic_set and smp_wmb in tx path Signed-off-by: Li RongQing <lirongqing@baidu.com> --- net/smc/smc_tx.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)