| Message ID | 20220826081117.21687-1-guoqing.jiang@linux.dev (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | rnbd-srv: remove 'dir' argument from rnbd_srv_rdma_ev | expand |
On Fri, Aug 26, 2022 at 10:11 AM Guoqing Jiang <guoqing.jiang@linux.dev> wrote: > > Since all callers (process_{read,write}) set id->dir, no need to > pass 'dir' again. > > Signed-off-by: Guoqing Jiang <guoqing.jiang@linux.dev> > --- > drivers/block/rnbd/rnbd-srv.c | 9 ++++----- > drivers/block/rnbd/rnbd-srv.h | 1 + > drivers/infiniband/ulp/rtrs/rtrs-srv.c | 4 ++-- > drivers/infiniband/ulp/rtrs/rtrs.h | 3 +-- > 4 files changed, 8 insertions(+), 9 deletions(-) > > diff --git a/drivers/block/rnbd/rnbd-srv.c b/drivers/block/rnbd/rnbd-srv.c > index 3f6c268e04ef..9600715f1029 100644 > --- a/drivers/block/rnbd/rnbd-srv.c > +++ b/drivers/block/rnbd/rnbd-srv.c > @@ -368,10 +368,9 @@ static int process_msg_sess_info(struct rnbd_srv_session *srv_sess, > const void *msg, size_t len, > void *data, size_t datalen); > > -static int rnbd_srv_rdma_ev(void *priv, > - struct rtrs_srv_op *id, int dir, > - void *data, size_t datalen, const void *usr, > - size_t usrlen) > +static int rnbd_srv_rdma_ev(void *priv, struct rtrs_srv_op *id, > + void *data, size_t datalen, > + const void *usr, size_t usrlen) > { > struct rnbd_srv_session *srv_sess = priv; > const struct rnbd_msg_hdr *hdr = usr; > @@ -398,7 +397,7 @@ static int rnbd_srv_rdma_ev(void *priv, > break; > default: > pr_warn("Received unexpected message type %d with dir %d from session %s\n", > - type, dir, srv_sess->sessname); > + type, id->dir, srv_sess->sessname); > return -EINVAL; > } > > diff --git a/drivers/block/rnbd/rnbd-srv.h b/drivers/block/rnbd/rnbd-srv.h > index 081bceaf4ae9..5a0ef6c2b5c7 100644 > --- a/drivers/block/rnbd/rnbd-srv.h > +++ b/drivers/block/rnbd/rnbd-srv.h > @@ -14,6 +14,7 @@ > #include <linux/kref.h> > > #include <rtrs.h> > +#include <rtrs-srv.h> why do we need this? > #include "rnbd-proto.h" > #include "rnbd-log.h" > > diff --git a/drivers/infiniband/ulp/rtrs/rtrs-srv.c b/drivers/infiniband/ulp/rtrs/rtrs-srv.c > index 34c03bde5064..9dc50ff0e1b9 100644 > --- a/drivers/infiniband/ulp/rtrs/rtrs-srv.c > +++ b/drivers/infiniband/ulp/rtrs/rtrs-srv.c > @@ -1024,7 +1024,7 @@ static void process_read(struct rtrs_srv_con *con, > usr_len = le16_to_cpu(msg->usr_len); > data_len = off - usr_len; > data = page_address(srv->chunks[buf_id]); > - ret = ctx->ops.rdma_ev(srv->priv, id, READ, data, data_len, > + ret = ctx->ops.rdma_ev(srv->priv, id, data, data_len, > data + data_len, usr_len); > > if (ret) { > @@ -1077,7 +1077,7 @@ static void process_write(struct rtrs_srv_con *con, > usr_len = le16_to_cpu(req->usr_len); > data_len = off - usr_len; > data = page_address(srv->chunks[buf_id]); > - ret = ctx->ops.rdma_ev(srv->priv, id, WRITE, data, data_len, > + ret = ctx->ops.rdma_ev(srv->priv, id, data, data_len, > data + data_len, usr_len); > if (ret) { > rtrs_err_rl(s, > diff --git a/drivers/infiniband/ulp/rtrs/rtrs.h b/drivers/infiniband/ulp/rtrs/rtrs.h > index 5e57a7ccc7fb..b48b53a7c143 100644 > --- a/drivers/infiniband/ulp/rtrs/rtrs.h > +++ b/drivers/infiniband/ulp/rtrs/rtrs.h > @@ -139,7 +139,6 @@ struct rtrs_srv_ops { > > * @priv: Private data set by rtrs_srv_set_sess_priv() > * @id: internal RTRS operation id > - * @dir: READ/WRITE > * @data: Pointer to (bidirectional) rdma memory area: > * - in case of %RTRS_SRV_RDMA_EV_RECV contains > * data sent by the client > @@ -151,7 +150,7 @@ struct rtrs_srv_ops { > * @usrlen: Size of the user message > */ > int (*rdma_ev)(void *priv, > - struct rtrs_srv_op *id, int dir, > + struct rtrs_srv_op *id, > void *data, size_t datalen, const void *usr, > size_t usrlen); > /** > -- > 2.31.1 >
On 8/26/22 6:48 PM, Jinpu Wang wrote: > On Fri, Aug 26, 2022 at 10:11 AM Guoqing Jiang <guoqing.jiang@linux.dev> wrote: >> Since all callers (process_{read,write}) set id->dir, no need to >> pass 'dir' again. >> >> Signed-off-by: Guoqing Jiang <guoqing.jiang@linux.dev> >> --- >> drivers/block/rnbd/rnbd-srv.c | 9 ++++----- >> drivers/block/rnbd/rnbd-srv.h | 1 + >> drivers/infiniband/ulp/rtrs/rtrs-srv.c | 4 ++-- >> drivers/infiniband/ulp/rtrs/rtrs.h | 3 +-- >> 4 files changed, 8 insertions(+), 9 deletions(-) >> >> diff --git a/drivers/block/rnbd/rnbd-srv.c b/drivers/block/rnbd/rnbd-srv.c >> index 3f6c268e04ef..9600715f1029 100644 >> --- a/drivers/block/rnbd/rnbd-srv.c >> +++ b/drivers/block/rnbd/rnbd-srv.c >> @@ -368,10 +368,9 @@ static int process_msg_sess_info(struct rnbd_srv_session *srv_sess, >> const void *msg, size_t len, >> void *data, size_t datalen); >> >> -static int rnbd_srv_rdma_ev(void *priv, >> - struct rtrs_srv_op *id, int dir, >> - void *data, size_t datalen, const void *usr, >> - size_t usrlen) >> +static int rnbd_srv_rdma_ev(void *priv, struct rtrs_srv_op *id, >> + void *data, size_t datalen, >> + const void *usr, size_t usrlen) >> { >> struct rnbd_srv_session *srv_sess = priv; >> const struct rnbd_msg_hdr *hdr = usr; >> @@ -398,7 +397,7 @@ static int rnbd_srv_rdma_ev(void *priv, >> break; >> default: >> pr_warn("Received unexpected message type %d with dir %d from session %s\n", >> - type, dir, srv_sess->sessname); >> + type, id->dir, srv_sess->sessname); >> return -EINVAL; >> } >> >> diff --git a/drivers/block/rnbd/rnbd-srv.h b/drivers/block/rnbd/rnbd-srv.h >> index 081bceaf4ae9..5a0ef6c2b5c7 100644 >> --- a/drivers/block/rnbd/rnbd-srv.h >> +++ b/drivers/block/rnbd/rnbd-srv.h >> @@ -14,6 +14,7 @@ >> #include <linux/kref.h> >> >> #include <rtrs.h> >> +#include <rtrs-srv.h> > why do we need this? Otherwise, compiler complains drivers/block/rnbd/rnbd-srv.c: In function ‘rnbd_srv_rdma_ev’: drivers/block/rnbd/rnbd-srv.c:400:33: error: invalid use of undefined type ‘struct rtrs_srv_op’ 400 | type, id->dir, srv_sess->sessname); Thanks, Guoqing
On Fri, Aug 26, 2022 at 1:26 PM Guoqing Jiang <guoqing.jiang@linux.dev> wrote: > > > > On 8/26/22 6:48 PM, Jinpu Wang wrote: > > On Fri, Aug 26, 2022 at 10:11 AM Guoqing Jiang <guoqing.jiang@linux.dev> wrote: > >> Since all callers (process_{read,write}) set id->dir, no need to > >> pass 'dir' again. > >> > >> Signed-off-by: Guoqing Jiang <guoqing.jiang@linux.dev> > >> --- > >> drivers/block/rnbd/rnbd-srv.c | 9 ++++----- > >> drivers/block/rnbd/rnbd-srv.h | 1 + > >> drivers/infiniband/ulp/rtrs/rtrs-srv.c | 4 ++-- > >> drivers/infiniband/ulp/rtrs/rtrs.h | 3 +-- > >> 4 files changed, 8 insertions(+), 9 deletions(-) > >> > >> diff --git a/drivers/block/rnbd/rnbd-srv.c b/drivers/block/rnbd/rnbd-srv.c > >> index 3f6c268e04ef..9600715f1029 100644 > >> --- a/drivers/block/rnbd/rnbd-srv.c > >> +++ b/drivers/block/rnbd/rnbd-srv.c > >> @@ -368,10 +368,9 @@ static int process_msg_sess_info(struct rnbd_srv_session *srv_sess, > >> const void *msg, size_t len, > >> void *data, size_t datalen); > >> > >> -static int rnbd_srv_rdma_ev(void *priv, > >> - struct rtrs_srv_op *id, int dir, > >> - void *data, size_t datalen, const void *usr, > >> - size_t usrlen) > >> +static int rnbd_srv_rdma_ev(void *priv, struct rtrs_srv_op *id, > >> + void *data, size_t datalen, > >> + const void *usr, size_t usrlen) > >> { > >> struct rnbd_srv_session *srv_sess = priv; > >> const struct rnbd_msg_hdr *hdr = usr; > >> @@ -398,7 +397,7 @@ static int rnbd_srv_rdma_ev(void *priv, > >> break; > >> default: > >> pr_warn("Received unexpected message type %d with dir %d from session %s\n", > >> - type, dir, srv_sess->sessname); > >> + type, id->dir, srv_sess->sessname); > >> return -EINVAL; > >> } > >> > >> diff --git a/drivers/block/rnbd/rnbd-srv.h b/drivers/block/rnbd/rnbd-srv.h > >> index 081bceaf4ae9..5a0ef6c2b5c7 100644 > >> --- a/drivers/block/rnbd/rnbd-srv.h > >> +++ b/drivers/block/rnbd/rnbd-srv.h > >> @@ -14,6 +14,7 @@ > >> #include <linux/kref.h> > >> > >> #include <rtrs.h> > >> +#include <rtrs-srv.h> > > why do we need this? > > Otherwise, compiler complains > > drivers/block/rnbd/rnbd-srv.c: In function ‘rnbd_srv_rdma_ev’: > drivers/block/rnbd/rnbd-srv.c:400:33: error: invalid use of undefined > type ‘struct rtrs_srv_op’ > 400 | type, id->dir, srv_sess->sessname); > > Thanks, > Guoqing ah, okay, this reminds me, why we have dir there, we don't want to export too much detail regarding the rtrs_srv_op to rnbd-server, it is supposed to be transparent to rnbd-srv.
On 8/26/22 7:29 PM, Jinpu Wang wrote: > On Fri, Aug 26, 2022 at 1:26 PM Guoqing Jiang <guoqing.jiang@linux.dev> wrote: >> >> >> On 8/26/22 6:48 PM, Jinpu Wang wrote: >>> On Fri, Aug 26, 2022 at 10:11 AM Guoqing Jiang <guoqing.jiang@linux.dev> wrote: >>>> Since all callers (process_{read,write}) set id->dir, no need to >>>> pass 'dir' again. >>>> >>>> Signed-off-by: Guoqing Jiang <guoqing.jiang@linux.dev> >>>> --- >>>> drivers/block/rnbd/rnbd-srv.c | 9 ++++----- >>>> drivers/block/rnbd/rnbd-srv.h | 1 + >>>> drivers/infiniband/ulp/rtrs/rtrs-srv.c | 4 ++-- >>>> drivers/infiniband/ulp/rtrs/rtrs.h | 3 +-- >>>> 4 files changed, 8 insertions(+), 9 deletions(-) >>>> >>>> diff --git a/drivers/block/rnbd/rnbd-srv.c b/drivers/block/rnbd/rnbd-srv.c >>>> index 3f6c268e04ef..9600715f1029 100644 >>>> --- a/drivers/block/rnbd/rnbd-srv.c >>>> +++ b/drivers/block/rnbd/rnbd-srv.c >>>> @@ -368,10 +368,9 @@ static int process_msg_sess_info(struct rnbd_srv_session *srv_sess, >>>> const void *msg, size_t len, >>>> void *data, size_t datalen); >>>> >>>> -static int rnbd_srv_rdma_ev(void *priv, >>>> - struct rtrs_srv_op *id, int dir, >>>> - void *data, size_t datalen, const void *usr, >>>> - size_t usrlen) >>>> +static int rnbd_srv_rdma_ev(void *priv, struct rtrs_srv_op *id, >>>> + void *data, size_t datalen, >>>> + const void *usr, size_t usrlen) >>>> { >>>> struct rnbd_srv_session *srv_sess = priv; >>>> const struct rnbd_msg_hdr *hdr = usr; >>>> @@ -398,7 +397,7 @@ static int rnbd_srv_rdma_ev(void *priv, >>>> break; >>>> default: >>>> pr_warn("Received unexpected message type %d with dir %d from session %s\n", >>>> - type, dir, srv_sess->sessname); >>>> + type, id->dir, srv_sess->sessname); >>>> return -EINVAL; >>>> } >>>> >>>> diff --git a/drivers/block/rnbd/rnbd-srv.h b/drivers/block/rnbd/rnbd-srv.h >>>> index 081bceaf4ae9..5a0ef6c2b5c7 100644 >>>> --- a/drivers/block/rnbd/rnbd-srv.h >>>> +++ b/drivers/block/rnbd/rnbd-srv.h >>>> @@ -14,6 +14,7 @@ >>>> #include <linux/kref.h> >>>> >>>> #include <rtrs.h> >>>> +#include <rtrs-srv.h> >>> why do we need this? >> Otherwise, compiler complains >> >> drivers/block/rnbd/rnbd-srv.c: In function ‘rnbd_srv_rdma_ev’: >> drivers/block/rnbd/rnbd-srv.c:400:33: error: invalid use of undefined >> type ‘struct rtrs_srv_op’ >> 400 | type, id->dir, srv_sess->sessname); >> >> Thanks, >> Guoqing > ah, okay, this reminds me, why we have dir there, we don't want to > export too much detail regarding the rtrs_srv_op to > rnbd-server, it is supposed to be transparent to rnbd-srv. What is the issue with more details are exported from rtrs-srv? Both of the modules are run in the same machine. And I guess we can just pass parameters with register after remove an argument, otherwise need to push/pop stack with more than six parameters for x64. Thanks, Guoqing
On Fri, Aug 26, 2022 at 1:38 PM Guoqing Jiang <guoqing.jiang@linux.dev> wrote: > > > > On 8/26/22 7:29 PM, Jinpu Wang wrote: > > On Fri, Aug 26, 2022 at 1:26 PM Guoqing Jiang <guoqing.jiang@linux.dev> wrote: > >> > >> > >> On 8/26/22 6:48 PM, Jinpu Wang wrote: > >>> On Fri, Aug 26, 2022 at 10:11 AM Guoqing Jiang <guoqing.jiang@linux.dev> wrote: > >>>> Since all callers (process_{read,write}) set id->dir, no need to > >>>> pass 'dir' again. > >>>> > >>>> Signed-off-by: Guoqing Jiang <guoqing.jiang@linux.dev> > >>>> --- > >>>> drivers/block/rnbd/rnbd-srv.c | 9 ++++----- > >>>> drivers/block/rnbd/rnbd-srv.h | 1 + > >>>> drivers/infiniband/ulp/rtrs/rtrs-srv.c | 4 ++-- > >>>> drivers/infiniband/ulp/rtrs/rtrs.h | 3 +-- > >>>> 4 files changed, 8 insertions(+), 9 deletions(-) > >>>> > >>>> diff --git a/drivers/block/rnbd/rnbd-srv.c b/drivers/block/rnbd/rnbd-srv.c > >>>> index 3f6c268e04ef..9600715f1029 100644 > >>>> --- a/drivers/block/rnbd/rnbd-srv.c > >>>> +++ b/drivers/block/rnbd/rnbd-srv.c > >>>> @@ -368,10 +368,9 @@ static int process_msg_sess_info(struct rnbd_srv_session *srv_sess, > >>>> const void *msg, size_t len, > >>>> void *data, size_t datalen); > >>>> > >>>> -static int rnbd_srv_rdma_ev(void *priv, > >>>> - struct rtrs_srv_op *id, int dir, > >>>> - void *data, size_t datalen, const void *usr, > >>>> - size_t usrlen) > >>>> +static int rnbd_srv_rdma_ev(void *priv, struct rtrs_srv_op *id, > >>>> + void *data, size_t datalen, > >>>> + const void *usr, size_t usrlen) > >>>> { > >>>> struct rnbd_srv_session *srv_sess = priv; > >>>> const struct rnbd_msg_hdr *hdr = usr; > >>>> @@ -398,7 +397,7 @@ static int rnbd_srv_rdma_ev(void *priv, > >>>> break; > >>>> default: > >>>> pr_warn("Received unexpected message type %d with dir %d from session %s\n", > >>>> - type, dir, srv_sess->sessname); > >>>> + type, id->dir, srv_sess->sessname); > >>>> return -EINVAL; > >>>> } > >>>> > >>>> diff --git a/drivers/block/rnbd/rnbd-srv.h b/drivers/block/rnbd/rnbd-srv.h > >>>> index 081bceaf4ae9..5a0ef6c2b5c7 100644 > >>>> --- a/drivers/block/rnbd/rnbd-srv.h > >>>> +++ b/drivers/block/rnbd/rnbd-srv.h > >>>> @@ -14,6 +14,7 @@ > >>>> #include <linux/kref.h> > >>>> > >>>> #include <rtrs.h> > >>>> +#include <rtrs-srv.h> > >>> why do we need this? > >> Otherwise, compiler complains > >> > >> drivers/block/rnbd/rnbd-srv.c: In function ‘rnbd_srv_rdma_ev’: > >> drivers/block/rnbd/rnbd-srv.c:400:33: error: invalid use of undefined > >> type ‘struct rtrs_srv_op’ > >> 400 | type, id->dir, srv_sess->sessname); > >> > >> Thanks, > >> Guoqing > > ah, okay, this reminds me, why we have dir there, we don't want to > > export too much detail regarding the rtrs_srv_op to > > rnbd-server, it is supposed to be transparent to rnbd-srv. > > What is the issue with more details are exported from rtrs-srv? Both of > the modules > are run in the same machine. with including rtrs-srv.h, the code size is bigger, not to mention many unnecessary info are exported to rnbd-srv module, ideally we want to have rnbd/rtrs layered properly with clear separation. > > And I guess we can just pass parameters with register after remove an > argument, > otherwise need to push/pop stack with more than six parameters for x64. I doubt it makes any notable performance change. > > Thanks, > Guoqing Thx!
On 8/26/22 7:58 PM, Jinpu Wang wrote: >> And I guess we can just pass parameters with register after remove an >> argument, >> otherwise need to push/pop stack with more than six parameters for x64. > I doubt it makes any notable performance change. I think it is called in the IO path (process_read and process_write) ... BTW, do you agree if the 'usr' can be dropped from rdma_ev given it is always same as 'data + data_len'? Thanks, Guoqing
On Fri, Aug 26, 2022 at 01:29:28PM +0200, Jinpu Wang wrote: > On Fri, Aug 26, 2022 at 1:26 PM Guoqing Jiang <guoqing.jiang@linux.dev> wrote: > > > > > > > > On 8/26/22 6:48 PM, Jinpu Wang wrote: > > > On Fri, Aug 26, 2022 at 10:11 AM Guoqing Jiang <guoqing.jiang@linux.dev> wrote: > > >> Since all callers (process_{read,write}) set id->dir, no need to > > >> pass 'dir' again. > > >> > > >> Signed-off-by: Guoqing Jiang <guoqing.jiang@linux.dev> > > >> --- > > >> drivers/block/rnbd/rnbd-srv.c | 9 ++++----- > > >> drivers/block/rnbd/rnbd-srv.h | 1 + > > >> drivers/infiniband/ulp/rtrs/rtrs-srv.c | 4 ++-- > > >> drivers/infiniband/ulp/rtrs/rtrs.h | 3 +-- > > >> 4 files changed, 8 insertions(+), 9 deletions(-) <...> > > >> #include <rtrs.h> > > >> +#include <rtrs-srv.h> > > > why do we need this? > > > > Otherwise, compiler complains > > > > drivers/block/rnbd/rnbd-srv.c: In function ‘rnbd_srv_rdma_ev’: > > drivers/block/rnbd/rnbd-srv.c:400:33: error: invalid use of undefined > > type ‘struct rtrs_srv_op’ > > 400 | type, id->dir, srv_sess->sessname); > > > > Thanks, > > Guoqing > ah, okay, this reminds me, why we have dir there, we don't want to > export too much detail regarding the rtrs_srv_op to > rnbd-server, it is supposed to be transparent to rnbd-srv. So decouple it from rtrs-srv.h and hide everything that not-needed to be exported to separate header file. Thanks
On Fri, Aug 26, 2022 at 04:11:17PM +0800, Guoqing Jiang wrote: > Since all callers (process_{read,write}) set id->dir, no need to > pass 'dir' again. > > Signed-off-by: Guoqing Jiang <guoqing.jiang@linux.dev> > --- > drivers/block/rnbd/rnbd-srv.c | 9 ++++----- > drivers/block/rnbd/rnbd-srv.h | 1 + > drivers/infiniband/ulp/rtrs/rtrs-srv.c | 4 ++-- > drivers/infiniband/ulp/rtrs/rtrs.h | 3 +-- > 4 files changed, 8 insertions(+), 9 deletions(-) I applied the patch and cleanup of rtrs-srv.h can be done later. Thanks
On Fri, Aug 26, 2022 at 4:02 PM Guoqing Jiang <guoqing.jiang@linux.dev> wrote: > > > > On 8/26/22 7:58 PM, Jinpu Wang wrote: > >> And I guess we can just pass parameters with register after remove an > >> argument, > >> otherwise need to push/pop stack with more than six parameters for x64. > > I doubt it makes any notable performance change. > > I think it is called in the IO path (process_read and process_write) ... > > BTW, do you agree if the 'usr' can be dropped from rdma_ev given it is > always > same as 'data + data_len'? in the current form, yes, usr can be dropped. we can add later if there is a need. > > Thanks, > Guoqing Thx!
On 8/29/22 3:44 PM, Leon Romanovsky wrote: > On Fri, Aug 26, 2022 at 04:11:17PM +0800, Guoqing Jiang wrote: >> Since all callers (process_{read,write}) set id->dir, no need to >> pass 'dir' again. >> >> Signed-off-by: Guoqing Jiang <guoqing.jiang@linux.dev> >> --- >> drivers/block/rnbd/rnbd-srv.c | 9 ++++----- >> drivers/block/rnbd/rnbd-srv.h | 1 + >> drivers/infiniband/ulp/rtrs/rtrs-srv.c | 4 ++-- >> drivers/infiniband/ulp/rtrs/rtrs.h | 3 +-- >> 4 files changed, 8 insertions(+), 9 deletions(-) > I applied the patch and cleanup of rtrs-srv.h can be done later. Thanks! I suppose below > So decouple it from rtrs-srv.h and hide everything that not-needed to be > exported to separate header file. means move 'struct rtrs_srv_op' to rtrs.h, which seems not appropriate to me because both client and server include the header. Pls correct me if I am wrong. Since process_{read,write} prints direction info if ctx->ops.rdma_ev fails, how about remove the 'dir' info from rnbd_srv_rdma_ev? Then we don't need to include rtrs-srv.h. diff --git a/drivers/block/rnbd/rnbd-srv.c b/drivers/block/rnbd/rnbd-srv.c index 431c6da19d3f..d07ff3ba560c 100644 --- a/drivers/block/rnbd/rnbd-srv.c +++ b/drivers/block/rnbd/rnbd-srv.c @@ -387,8 +387,8 @@ static int rnbd_srv_rdma_ev(void *priv, struct rtrs_srv_op *id, datalen); break; default: - pr_warn("Received unexpected message type %d with dir %d from session %s\n", - type, id->dir, srv_sess->sessname); + pr_warn("Received unexpected message type %d from session %s\n", + type, srv_sess->sessname); return -EINVAL; } diff --git a/drivers/block/rnbd/rnbd-srv.h b/drivers/block/rnbd/rnbd-srv.h index 5a0ef6c2b5c7..081bceaf4ae9 100644 --- a/drivers/block/rnbd/rnbd-srv.h +++ b/drivers/block/rnbd/rnbd-srv.h @@ -14,7 +14,6 @@ #include <linux/kref.h> #include <rtrs.h> -#include <rtrs-srv.h> #include "rnbd-proto.h" #include "rnbd-log.h" Thoughts? Thanks, Guoqing
On Mon, Aug 29, 2022 at 3:33 PM Guoqing Jiang <guoqing.jiang@linux.dev> wrote: > > > > On 8/29/22 3:44 PM, Leon Romanovsky wrote: > > On Fri, Aug 26, 2022 at 04:11:17PM +0800, Guoqing Jiang wrote: > >> Since all callers (process_{read,write}) set id->dir, no need to > >> pass 'dir' again. > >> > >> Signed-off-by: Guoqing Jiang <guoqing.jiang@linux.dev> > >> --- > >> drivers/block/rnbd/rnbd-srv.c | 9 ++++----- > >> drivers/block/rnbd/rnbd-srv.h | 1 + > >> drivers/infiniband/ulp/rtrs/rtrs-srv.c | 4 ++-- > >> drivers/infiniband/ulp/rtrs/rtrs.h | 3 +-- > >> 4 files changed, 8 insertions(+), 9 deletions(-) > > > I applied the patch and cleanup of rtrs-srv.h can be done later. > > Thanks! I suppose below > > > So decouple it from rtrs-srv.h and hide everything that not-needed to be > > exported to separate header file. > > means move 'struct rtrs_srv_op' to rtrs.h, which seems not appropriate to me > because both client and server include the header. Pls correct me if I > am wrong. > > Since process_{read,write} prints direction info if ctx->ops.rdma_ev > fails, how > about remove the 'dir' info from rnbd_srv_rdma_ev? Then we don't need to > include rtrs-srv.h. > > diff --git a/drivers/block/rnbd/rnbd-srv.c b/drivers/block/rnbd/rnbd-srv.c > index 431c6da19d3f..d07ff3ba560c 100644 > --- a/drivers/block/rnbd/rnbd-srv.c > +++ b/drivers/block/rnbd/rnbd-srv.c > @@ -387,8 +387,8 @@ static int rnbd_srv_rdma_ev(void *priv, struct > rtrs_srv_op *id, > datalen); > break; > default: > - pr_warn("Received unexpected message type %d with dir %d > from session %s\n", > - type, id->dir, srv_sess->sessname); > + pr_warn("Received unexpected message type %d from > session %s\n", > + type, srv_sess->sessname); > return -EINVAL; > } > > diff --git a/drivers/block/rnbd/rnbd-srv.h b/drivers/block/rnbd/rnbd-srv.h > index 5a0ef6c2b5c7..081bceaf4ae9 100644 > --- a/drivers/block/rnbd/rnbd-srv.h > +++ b/drivers/block/rnbd/rnbd-srv.h > @@ -14,7 +14,6 @@ > #include <linux/kref.h> > > #include <rtrs.h> > -#include <rtrs-srv.h> > #include "rnbd-proto.h" > #include "rnbd-log.h" > > > Thoughts? I like the idea. Please post a formal patch based on leon's https://git.kernel.org/pub/scm/linux/kernel/git/rdma/rdma.git/log/?h=wip/leon-for-next Thx! > > Thanks, > Guoqing
On Mon, Aug 29, 2022 at 03:43:43PM +0200, Jinpu Wang wrote: > On Mon, Aug 29, 2022 at 3:33 PM Guoqing Jiang <guoqing.jiang@linux.dev> wrote: > > > > > > > > On 8/29/22 3:44 PM, Leon Romanovsky wrote: > > > On Fri, Aug 26, 2022 at 04:11:17PM +0800, Guoqing Jiang wrote: > > >> Since all callers (process_{read,write}) set id->dir, no need to > > >> pass 'dir' again. > > >> > > >> Signed-off-by: Guoqing Jiang <guoqing.jiang@linux.dev> > > >> --- > > >> drivers/block/rnbd/rnbd-srv.c | 9 ++++----- > > >> drivers/block/rnbd/rnbd-srv.h | 1 + > > >> drivers/infiniband/ulp/rtrs/rtrs-srv.c | 4 ++-- > > >> drivers/infiniband/ulp/rtrs/rtrs.h | 3 +-- > > >> 4 files changed, 8 insertions(+), 9 deletions(-) > > > > > I applied the patch and cleanup of rtrs-srv.h can be done later. > > > > Thanks! I suppose below > > > > > So decouple it from rtrs-srv.h and hide everything that not-needed to be > > > exported to separate header file. > > > > means move 'struct rtrs_srv_op' to rtrs.h, which seems not appropriate to me > > because both client and server include the header. Pls correct me if I > > am wrong. > > > > Since process_{read,write} prints direction info if ctx->ops.rdma_ev > > fails, how > > about remove the 'dir' info from rnbd_srv_rdma_ev? Then we don't need to > > include rtrs-srv.h. > > > > diff --git a/drivers/block/rnbd/rnbd-srv.c b/drivers/block/rnbd/rnbd-srv.c > > index 431c6da19d3f..d07ff3ba560c 100644 > > --- a/drivers/block/rnbd/rnbd-srv.c > > +++ b/drivers/block/rnbd/rnbd-srv.c > > @@ -387,8 +387,8 @@ static int rnbd_srv_rdma_ev(void *priv, struct > > rtrs_srv_op *id, > > datalen); > > break; > > default: > > - pr_warn("Received unexpected message type %d with dir %d > > from session %s\n", > > - type, id->dir, srv_sess->sessname); > > + pr_warn("Received unexpected message type %d from > > session %s\n", > > + type, srv_sess->sessname); > > return -EINVAL; > > } > > > > diff --git a/drivers/block/rnbd/rnbd-srv.h b/drivers/block/rnbd/rnbd-srv.h > > index 5a0ef6c2b5c7..081bceaf4ae9 100644 > > --- a/drivers/block/rnbd/rnbd-srv.h > > +++ b/drivers/block/rnbd/rnbd-srv.h > > @@ -14,7 +14,6 @@ > > #include <linux/kref.h> > > > > #include <rtrs.h> > > -#include <rtrs-srv.h> > > #include "rnbd-proto.h" > > #include "rnbd-log.h" > > > > > > Thoughts? > I like the idea. Please post a formal patch based on leon's > https://git.kernel.org/pub/scm/linux/kernel/git/rdma/rdma.git/log/?h=wip/leon-for-next I squashed this hunk into original patch. Thanks > > Thx! > > > > Thanks, > > Guoqing
On 8/30/22 4:44 PM, Leon Romanovsky wrote: > On Mon, Aug 29, 2022 at 03:43:43PM +0200, Jinpu Wang wrote: >> On Mon, Aug 29, 2022 at 3:33 PM Guoqing Jiang <guoqing.jiang@linux.dev> wrote: >>> >>> >>> On 8/29/22 3:44 PM, Leon Romanovsky wrote: >>>> On Fri, Aug 26, 2022 at 04:11:17PM +0800, Guoqing Jiang wrote: >>>>> Since all callers (process_{read,write}) set id->dir, no need to >>>>> pass 'dir' again. >>>>> >>>>> Signed-off-by: Guoqing Jiang <guoqing.jiang@linux.dev> >>>>> --- >>>>> drivers/block/rnbd/rnbd-srv.c | 9 ++++----- >>>>> drivers/block/rnbd/rnbd-srv.h | 1 + >>>>> drivers/infiniband/ulp/rtrs/rtrs-srv.c | 4 ++-- >>>>> drivers/infiniband/ulp/rtrs/rtrs.h | 3 +-- >>>>> 4 files changed, 8 insertions(+), 9 deletions(-) >>>> I applied the patch and cleanup of rtrs-srv.h can be done later. >>> Thanks! I suppose below >>> >>>> So decouple it from rtrs-srv.h and hide everything that not-needed to be >>>> exported to separate header file. >>> means move 'struct rtrs_srv_op' to rtrs.h, which seems not appropriate to me >>> because both client and server include the header. Pls correct me if I >>> am wrong. >>> >>> Since process_{read,write} prints direction info if ctx->ops.rdma_ev >>> fails, how >>> about remove the 'dir' info from rnbd_srv_rdma_ev? Then we don't need to >>> include rtrs-srv.h. >>> >>> diff --git a/drivers/block/rnbd/rnbd-srv.c b/drivers/block/rnbd/rnbd-srv.c >>> index 431c6da19d3f..d07ff3ba560c 100644 >>> --- a/drivers/block/rnbd/rnbd-srv.c >>> +++ b/drivers/block/rnbd/rnbd-srv.c >>> @@ -387,8 +387,8 @@ static int rnbd_srv_rdma_ev(void *priv, struct >>> rtrs_srv_op *id, >>> datalen); >>> break; >>> default: >>> - pr_warn("Received unexpected message type %d with dir %d >>> from session %s\n", >>> - type, id->dir, srv_sess->sessname); >>> + pr_warn("Received unexpected message type %d from >>> session %s\n", >>> + type, srv_sess->sessname); >>> return -EINVAL; >>> } >>> >>> diff --git a/drivers/block/rnbd/rnbd-srv.h b/drivers/block/rnbd/rnbd-srv.h >>> index 5a0ef6c2b5c7..081bceaf4ae9 100644 >>> --- a/drivers/block/rnbd/rnbd-srv.h >>> +++ b/drivers/block/rnbd/rnbd-srv.h >>> @@ -14,7 +14,6 @@ >>> #include <linux/kref.h> >>> >>> #include <rtrs.h> >>> -#include <rtrs-srv.h> >>> #include "rnbd-proto.h" >>> #include "rnbd-log.h" >>> >>> >>> Thoughts? >> I like the idea. Please post a formal patch based on leon's >> https://git.kernel.org/pub/scm/linux/kernel/git/rdma/rdma.git/log/?h=wip/leon-for-next > I squashed this hunk into original patch. Great, thank you! If possible, to better reflect the change, please at your convenience to replace the original commit message with below. "Since process_{read,write} already prints direction info if ctx->ops.rdma_ev fails, no need to pass 'dir'" Thanks, Guoqing
On Tue, Aug 30, 2022 at 10:44 AM Leon Romanovsky <leon@kernel.org> wrote: > > On Mon, Aug 29, 2022 at 03:43:43PM +0200, Jinpu Wang wrote: > > On Mon, Aug 29, 2022 at 3:33 PM Guoqing Jiang <guoqing.jiang@linux.dev> wrote: > > > > > > > > > > > > On 8/29/22 3:44 PM, Leon Romanovsky wrote: > > > > On Fri, Aug 26, 2022 at 04:11:17PM +0800, Guoqing Jiang wrote: > > > >> Since all callers (process_{read,write}) set id->dir, no need to > > > >> pass 'dir' again. > > > >> > > > >> Signed-off-by: Guoqing Jiang <guoqing.jiang@linux.dev> > > > >> --- > > > >> drivers/block/rnbd/rnbd-srv.c | 9 ++++----- > > > >> drivers/block/rnbd/rnbd-srv.h | 1 + > > > >> drivers/infiniband/ulp/rtrs/rtrs-srv.c | 4 ++-- > > > >> drivers/infiniband/ulp/rtrs/rtrs.h | 3 +-- > > > >> 4 files changed, 8 insertions(+), 9 deletions(-) > > > > > > > I applied the patch and cleanup of rtrs-srv.h can be done later. > > > > > > Thanks! I suppose below > > > > > > > So decouple it from rtrs-srv.h and hide everything that not-needed to be > > > > exported to separate header file. > > > > > > means move 'struct rtrs_srv_op' to rtrs.h, which seems not appropriate to me > > > because both client and server include the header. Pls correct me if I > > > am wrong. > > > > > > Since process_{read,write} prints direction info if ctx->ops.rdma_ev > > > fails, how > > > about remove the 'dir' info from rnbd_srv_rdma_ev? Then we don't need to > > > include rtrs-srv.h. > > > > > > diff --git a/drivers/block/rnbd/rnbd-srv.c b/drivers/block/rnbd/rnbd-srv.c > > > index 431c6da19d3f..d07ff3ba560c 100644 > > > --- a/drivers/block/rnbd/rnbd-srv.c > > > +++ b/drivers/block/rnbd/rnbd-srv.c > > > @@ -387,8 +387,8 @@ static int rnbd_srv_rdma_ev(void *priv, struct > > > rtrs_srv_op *id, > > > datalen); > > > break; > > > default: > > > - pr_warn("Received unexpected message type %d with dir %d > > > from session %s\n", > > > - type, id->dir, srv_sess->sessname); > > > + pr_warn("Received unexpected message type %d from > > > session %s\n", > > > + type, srv_sess->sessname); > > > return -EINVAL; > > > } > > > > > > diff --git a/drivers/block/rnbd/rnbd-srv.h b/drivers/block/rnbd/rnbd-srv.h > > > index 5a0ef6c2b5c7..081bceaf4ae9 100644 > > > --- a/drivers/block/rnbd/rnbd-srv.h > > > +++ b/drivers/block/rnbd/rnbd-srv.h > > > @@ -14,7 +14,6 @@ > > > #include <linux/kref.h> > > > > > > #include <rtrs.h> > > > -#include <rtrs-srv.h> > > > #include "rnbd-proto.h" > > > #include "rnbd-log.h" > > > > > > > > > Thoughts? > > I like the idea. Please post a formal patch based on leon's > > https://git.kernel.org/pub/scm/linux/kernel/git/rdma/rdma.git/log/?h=wip/leon-for-next > > I squashed this hunk into original patch. > Hi Leon, Thank you! > Thanks > > > > > Thx! > > > > > > Thanks, > > > Guoqing
On Tue, Aug 30, 2022 at 04:57:19PM +0800, Guoqing Jiang wrote: > > > On 8/30/22 4:44 PM, Leon Romanovsky wrote: > > On Mon, Aug 29, 2022 at 03:43:43PM +0200, Jinpu Wang wrote: > > > On Mon, Aug 29, 2022 at 3:33 PM Guoqing Jiang <guoqing.jiang@linux.dev> wrote: > > > > > > > > > > > > On 8/29/22 3:44 PM, Leon Romanovsky wrote: > > > > > On Fri, Aug 26, 2022 at 04:11:17PM +0800, Guoqing Jiang wrote: > > > > > > Since all callers (process_{read,write}) set id->dir, no need to > > > > > > pass 'dir' again. > > > > > > > > > > > > Signed-off-by: Guoqing Jiang <guoqing.jiang@linux.dev> > > > > > > --- > > > > > > drivers/block/rnbd/rnbd-srv.c | 9 ++++----- > > > > > > drivers/block/rnbd/rnbd-srv.h | 1 + > > > > > > drivers/infiniband/ulp/rtrs/rtrs-srv.c | 4 ++-- > > > > > > drivers/infiniband/ulp/rtrs/rtrs.h | 3 +-- > > > > > > 4 files changed, 8 insertions(+), 9 deletions(-) > > > > > I applied the patch and cleanup of rtrs-srv.h can be done later. > > > > Thanks! I suppose below > > > > > > > > > So decouple it from rtrs-srv.h and hide everything that not-needed to be > > > > > exported to separate header file. > > > > means move 'struct rtrs_srv_op' to rtrs.h, which seems not appropriate to me > > > > because both client and server include the header. Pls correct me if I > > > > am wrong. > > > > > > > > Since process_{read,write} prints direction info if ctx->ops.rdma_ev > > > > fails, how > > > > about remove the 'dir' info from rnbd_srv_rdma_ev? Then we don't need to > > > > include rtrs-srv.h. > > > > > > > > diff --git a/drivers/block/rnbd/rnbd-srv.c b/drivers/block/rnbd/rnbd-srv.c > > > > index 431c6da19d3f..d07ff3ba560c 100644 > > > > --- a/drivers/block/rnbd/rnbd-srv.c > > > > +++ b/drivers/block/rnbd/rnbd-srv.c > > > > @@ -387,8 +387,8 @@ static int rnbd_srv_rdma_ev(void *priv, struct > > > > rtrs_srv_op *id, > > > > datalen); > > > > break; > > > > default: > > > > - pr_warn("Received unexpected message type %d with dir %d > > > > from session %s\n", > > > > - type, id->dir, srv_sess->sessname); > > > > + pr_warn("Received unexpected message type %d from > > > > session %s\n", > > > > + type, srv_sess->sessname); > > > > return -EINVAL; > > > > } > > > > > > > > diff --git a/drivers/block/rnbd/rnbd-srv.h b/drivers/block/rnbd/rnbd-srv.h > > > > index 5a0ef6c2b5c7..081bceaf4ae9 100644 > > > > --- a/drivers/block/rnbd/rnbd-srv.h > > > > +++ b/drivers/block/rnbd/rnbd-srv.h > > > > @@ -14,7 +14,6 @@ > > > > #include <linux/kref.h> > > > > > > > > #include <rtrs.h> > > > > -#include <rtrs-srv.h> > > > > #include "rnbd-proto.h" > > > > #include "rnbd-log.h" > > > > > > > > > > > > Thoughts? > > > I like the idea. Please post a formal patch based on leon's > > > https://git.kernel.org/pub/scm/linux/kernel/git/rdma/rdma.git/log/?h=wip/leon-for-next > > I squashed this hunk into original patch. > > Great, thank you! > > If possible, to better reflect the change, please at your convenience to > replace the original commit > message with below. > > "Since process_{read,write} already prints direction info if > ctx->ops.rdma_ev fails, no need to pass 'dir'" Done, as long as patches are in wip/* branches, we can rebase them. Thanks > > Thanks, > Guoqing
diff --git a/drivers/block/rnbd/rnbd-srv.c b/drivers/block/rnbd/rnbd-srv.c index 3f6c268e04ef..9600715f1029 100644 --- a/drivers/block/rnbd/rnbd-srv.c +++ b/drivers/block/rnbd/rnbd-srv.c @@ -368,10 +368,9 @@ static int process_msg_sess_info(struct rnbd_srv_session *srv_sess, const void *msg, size_t len, void *data, size_t datalen); -static int rnbd_srv_rdma_ev(void *priv, - struct rtrs_srv_op *id, int dir, - void *data, size_t datalen, const void *usr, - size_t usrlen) +static int rnbd_srv_rdma_ev(void *priv, struct rtrs_srv_op *id, + void *data, size_t datalen, + const void *usr, size_t usrlen) { struct rnbd_srv_session *srv_sess = priv; const struct rnbd_msg_hdr *hdr = usr; @@ -398,7 +397,7 @@ static int rnbd_srv_rdma_ev(void *priv, break; default: pr_warn("Received unexpected message type %d with dir %d from session %s\n", - type, dir, srv_sess->sessname); + type, id->dir, srv_sess->sessname); return -EINVAL; } diff --git a/drivers/block/rnbd/rnbd-srv.h b/drivers/block/rnbd/rnbd-srv.h index 081bceaf4ae9..5a0ef6c2b5c7 100644 --- a/drivers/block/rnbd/rnbd-srv.h +++ b/drivers/block/rnbd/rnbd-srv.h @@ -14,6 +14,7 @@ #include <linux/kref.h> #include <rtrs.h> +#include <rtrs-srv.h> #include "rnbd-proto.h" #include "rnbd-log.h" diff --git a/drivers/infiniband/ulp/rtrs/rtrs-srv.c b/drivers/infiniband/ulp/rtrs/rtrs-srv.c index 34c03bde5064..9dc50ff0e1b9 100644 --- a/drivers/infiniband/ulp/rtrs/rtrs-srv.c +++ b/drivers/infiniband/ulp/rtrs/rtrs-srv.c @@ -1024,7 +1024,7 @@ static void process_read(struct rtrs_srv_con *con, usr_len = le16_to_cpu(msg->usr_len); data_len = off - usr_len; data = page_address(srv->chunks[buf_id]); - ret = ctx->ops.rdma_ev(srv->priv, id, READ, data, data_len, + ret = ctx->ops.rdma_ev(srv->priv, id, data, data_len, data + data_len, usr_len); if (ret) { @@ -1077,7 +1077,7 @@ static void process_write(struct rtrs_srv_con *con, usr_len = le16_to_cpu(req->usr_len); data_len = off - usr_len; data = page_address(srv->chunks[buf_id]); - ret = ctx->ops.rdma_ev(srv->priv, id, WRITE, data, data_len, + ret = ctx->ops.rdma_ev(srv->priv, id, data, data_len, data + data_len, usr_len); if (ret) { rtrs_err_rl(s, diff --git a/drivers/infiniband/ulp/rtrs/rtrs.h b/drivers/infiniband/ulp/rtrs/rtrs.h index 5e57a7ccc7fb..b48b53a7c143 100644 --- a/drivers/infiniband/ulp/rtrs/rtrs.h +++ b/drivers/infiniband/ulp/rtrs/rtrs.h @@ -139,7 +139,6 @@ struct rtrs_srv_ops { * @priv: Private data set by rtrs_srv_set_sess_priv() * @id: internal RTRS operation id - * @dir: READ/WRITE * @data: Pointer to (bidirectional) rdma memory area: * - in case of %RTRS_SRV_RDMA_EV_RECV contains * data sent by the client @@ -151,7 +150,7 @@ struct rtrs_srv_ops { * @usrlen: Size of the user message */ int (*rdma_ev)(void *priv, - struct rtrs_srv_op *id, int dir, + struct rtrs_srv_op *id, void *data, size_t datalen, const void *usr, size_t usrlen); /**
Since all callers (process_{read,write}) set id->dir, no need to pass 'dir' again. Signed-off-by: Guoqing Jiang <guoqing.jiang@linux.dev> --- drivers/block/rnbd/rnbd-srv.c | 9 ++++----- drivers/block/rnbd/rnbd-srv.h | 1 + drivers/infiniband/ulp/rtrs/rtrs-srv.c | 4 ++-- drivers/infiniband/ulp/rtrs/rtrs.h | 3 +-- 4 files changed, 8 insertions(+), 9 deletions(-)