| Message ID | 20200527081742.25718-1-rao.shoaib@oracle.com |
|---|---|
| State | Rejected |
| Delegated to: | David Miller |
| Headers | show |
| Series | [net-next] rds: transport module should be auto loaded when transport is set | expand |
From: rao.shoaib@oracle.com Date: Wed, 27 May 2020 01:17:42 -0700 > diff --git a/include/uapi/linux/rds.h b/include/uapi/linux/rds.h > index cba368e55863..7273c681e6c1 100644 > --- a/include/uapi/linux/rds.h > +++ b/include/uapi/linux/rds.h > @@ -64,7 +64,7 @@ > > /* supported values for SO_RDS_TRANSPORT */ > #define RDS_TRANS_IB 0 > -#define RDS_TRANS_IWARP 1 > +#define RDS_TRANS_GAP 1 > #define RDS_TRANS_TCP 2 > #define RDS_TRANS_COUNT 3 > #define RDS_TRANS_NONE (~0) You can't break user facing UAPI like this, sorry.
On Wed, May 27, 2020 at 01:17:42AM -0700, rao.shoaib@oracle.com wrote: > From: Rao Shoaib <rao.shoaib@oracle.com> > > This enhancement auto loads transport module when the transport > is set via SO_RDS_TRANSPORT socket option. > > Orabug: 31032127 I think that it is internal to Oracle and should not be in the commit message. Thanks
On 5/29/20 4:41 PM, David Miller wrote: > From: rao.shoaib@oracle.com > Date: Wed, 27 May 2020 01:17:42 -0700 > >> diff --git a/include/uapi/linux/rds.h b/include/uapi/linux/rds.h >> index cba368e55863..7273c681e6c1 100644 >> --- a/include/uapi/linux/rds.h >> +++ b/include/uapi/linux/rds.h >> @@ -64,7 +64,7 @@ >> >> /* supported values for SO_RDS_TRANSPORT */ >> #define RDS_TRANS_IB 0 >> -#define RDS_TRANS_IWARP 1 >> +#define RDS_TRANS_GAP 1 >> #define RDS_TRANS_TCP 2 >> #define RDS_TRANS_COUNT 3 >> #define RDS_TRANS_NONE (~0) > You can't break user facing UAPI like this, sorry. I was hoping that this could be considered an exception as IWARP has been deprecated for almost a decade and there is no current product using it. With the change any old binary will continue to work, a new compilation fill fail so that the code can be examined, otherwise we will never be able to reuse this number. If the above is not acceptable I can revert this part of the change. Shoaib
On 5/31/20 3:08 AM, Leon Romanovsky wrote: > On Wed, May 27, 2020 at 01:17:42AM -0700, rao.shoaib@oracle.com wrote: >> From: Rao Shoaib <rao.shoaib@oracle.com> >> >> This enhancement auto loads transport module when the transport >> is set via SO_RDS_TRANSPORT socket option. >> >> Orabug: 31032127 > I think that it is internal to Oracle and should not be in the commit > message. > > Thanks There are logs that have internals bug numbers mentioned in them. I do agree with you and will take the bug number out. Thanks for the comment. Shoaib
On Mon, Jun 01, 2020 at 09:59:30AM -0700, Rao Shoaib wrote: > > On 5/29/20 4:41 PM, David Miller wrote: > > From: rao.shoaib@oracle.com > > Date: Wed, 27 May 2020 01:17:42 -0700 > > > > > diff --git a/include/uapi/linux/rds.h b/include/uapi/linux/rds.h > > > index cba368e55863..7273c681e6c1 100644 > > > --- a/include/uapi/linux/rds.h > > > +++ b/include/uapi/linux/rds.h > > > @@ -64,7 +64,7 @@ > > > /* supported values for SO_RDS_TRANSPORT */ > > > #define RDS_TRANS_IB 0 > > > -#define RDS_TRANS_IWARP 1 > > > +#define RDS_TRANS_GAP 1 > > > #define RDS_TRANS_TCP 2 > > > #define RDS_TRANS_COUNT 3 > > > #define RDS_TRANS_NONE (~0) > > You can't break user facing UAPI like this, sorry. > > I was hoping that this could be considered an exception as IWARP has been > deprecated for almost a decade and there is no current product using it. > With the change any old binary will continue to work, a new compilation fill > fail so that the code can be examined, otherwise we will never be able to > reuse this number. > > If the above is not acceptable I can revert this part of the change. Nothing prohibits you from adding the following lines: + /* don't use RDS_TRANS_IWARP - it is deprecated */ + #define RDS_TRANS_GAP RDS_TRANS_IWARP > > Shoaib >
On Mon, Jun 01, 2020 at 10:08:44AM -0700, Rao Shoaib wrote: > > On 5/31/20 3:08 AM, Leon Romanovsky wrote: > > On Wed, May 27, 2020 at 01:17:42AM -0700, rao.shoaib@oracle.com wrote: > > > From: Rao Shoaib <rao.shoaib@oracle.com> > > > > > > This enhancement auto loads transport module when the transport > > > is set via SO_RDS_TRANSPORT socket option. > > > > > > Orabug: 31032127 > > I think that it is internal to Oracle and should not be in the commit > > message. > > > > Thanks > > There are logs that have internals bug numbers mentioned in them. Right, it was missed in the review. > I do agree with you and will take the bug number out. > > Thanks for the comment. > > Shoaib >
On 6/1/20 11:17 PM, Leon Romanovsky wrote: > On Mon, Jun 01, 2020 at 09:59:30AM -0700, Rao Shoaib wrote: >> On 5/29/20 4:41 PM, David Miller wrote: >>> From: rao.shoaib@oracle.com >>> Date: Wed, 27 May 2020 01:17:42 -0700 >>> >>>> diff --git a/include/uapi/linux/rds.h b/include/uapi/linux/rds.h >>>> index cba368e55863..7273c681e6c1 100644 >>>> --- a/include/uapi/linux/rds.h >>>> +++ b/include/uapi/linux/rds.h >>>> @@ -64,7 +64,7 @@ >>>> /* supported values for SO_RDS_TRANSPORT */ >>>> #define RDS_TRANS_IB 0 >>>> -#define RDS_TRANS_IWARP 1 >>>> +#define RDS_TRANS_GAP 1 >>>> #define RDS_TRANS_TCP 2 >>>> #define RDS_TRANS_COUNT 3 >>>> #define RDS_TRANS_NONE (~0) >>> You can't break user facing UAPI like this, sorry. >> I was hoping that this could be considered an exception as IWARP has been >> deprecated for almost a decade and there is no current product using it. >> With the change any old binary will continue to work, a new compilation fill >> fail so that the code can be examined, otherwise we will never be able to >> reuse this number. >> >> If the above is not acceptable I can revert this part of the change. > Nothing prohibits you from adding the following lines: > > + /* don't use RDS_TRANS_IWARP - it is deprecated */ > + #define RDS_TRANS_GAP RDS_TRANS_IWARP Correct. That is what I was planning on doing in case I could not get rid of RDS_TRANS_IWARP. I will resubmit the patch. Shoaib > >> Shoaib >>
diff --git a/include/uapi/linux/rds.h b/include/uapi/linux/rds.h index cba368e55863..7273c681e6c1 100644 --- a/include/uapi/linux/rds.h +++ b/include/uapi/linux/rds.h @@ -64,7 +64,7 @@ /* supported values for SO_RDS_TRANSPORT */ #define RDS_TRANS_IB 0 -#define RDS_TRANS_IWARP 1 +#define RDS_TRANS_GAP 1 #define RDS_TRANS_TCP 2 #define RDS_TRANS_COUNT 3 #define RDS_TRANS_NONE (~0) diff --git a/net/rds/transport.c b/net/rds/transport.c index 46f709a4b577..f8001ec80867 100644 --- a/net/rds/transport.c +++ b/net/rds/transport.c @@ -38,6 +38,12 @@ #include "rds.h" #include "loop.h" +static char * const rds_trans_modules[] = { + [RDS_TRANS_IB] = "rds_rdma", + [RDS_TRANS_GAP] = NULL, + [RDS_TRANS_TCP] = "rds_tcp", +}; + static struct rds_transport *transports[RDS_TRANS_COUNT]; static DECLARE_RWSEM(rds_trans_sem); @@ -110,18 +116,20 @@ struct rds_transport *rds_trans_get(int t_type) { struct rds_transport *ret = NULL; struct rds_transport *trans; - unsigned int i; down_read(&rds_trans_sem); - for (i = 0; i < RDS_TRANS_COUNT; i++) { - trans = transports[i]; - - if (trans && trans->t_type == t_type && - (!trans->t_owner || try_module_get(trans->t_owner))) { - ret = trans; - break; - } + trans = transports[t_type]; + if (!trans) { + up_read(&rds_trans_sem); + if (rds_trans_modules[t_type]) + request_module(rds_trans_modules[t_type]); + down_read(&rds_trans_sem); + trans = transports[t_type]; } + if (trans && trans->t_type == t_type && + (!trans->t_owner || try_module_get(trans->t_owner))) + ret = trans; + up_read(&rds_trans_sem); return ret;