Message ID | 1481083316-11648-19-git-send-email-santosh.shilimkar@oracle.com |
---|---|
State | Changes Requested, archived |
Delegated to: | David Miller |
Headers | show |
From: Santosh Shilimkar <santosh.shilimkar@oracle.com> Date: Tue, 6 Dec 2016 20:01:56 -0800 > rds-tools already support it. > > Signed-off-by: Santosh Shilimkar <santosh.shilimkar@oracle.com> > --- > include/uapi/linux/rds.h | 1 + > net/rds/ib.c | 1 + > 2 files changed, 2 insertions(+) > > diff --git a/include/uapi/linux/rds.h b/include/uapi/linux/rds.h > index 3833113..410ae3c 100644 > --- a/include/uapi/linux/rds.h > +++ b/include/uapi/linux/rds.h > @@ -183,6 +183,7 @@ struct rds_info_rdma_connection { > uint32_t max_send_sge; > uint32_t rdma_mr_max; > uint32_t rdma_mr_size; > + uint32_t cache_allocs; > }; > > /* RDS message Receive Path Latency points */ What level of compatability exists here? If we run an old tool on a new kernel, or a new tool on an old kernel, does it work properly?
On 12/7/2016 7:55 AM, David Miller wrote: > From: Santosh Shilimkar <santosh.shilimkar@oracle.com> > Date: Tue, 6 Dec 2016 20:01:56 -0800 > >> rds-tools already support it. >> >> Signed-off-by: Santosh Shilimkar <santosh.shilimkar@oracle.com> >> --- >> include/uapi/linux/rds.h | 1 + >> net/rds/ib.c | 1 + >> 2 files changed, 2 insertions(+) >> >> diff --git a/include/uapi/linux/rds.h b/include/uapi/linux/rds.h >> index 3833113..410ae3c 100644 >> --- a/include/uapi/linux/rds.h >> +++ b/include/uapi/linux/rds.h >> @@ -183,6 +183,7 @@ struct rds_info_rdma_connection { >> uint32_t max_send_sge; >> uint32_t rdma_mr_max; >> uint32_t rdma_mr_size; >> + uint32_t cache_allocs; >> }; >> >> /* RDS message Receive Path Latency points */ > > What level of compatability exists here? If we run an old tool on a new > kernel, or a new tool on an old kernel, does it work properly? > Tools repo carries a copy of the header and thats how the old tool and new tools have been running with older/newer kernels. There are few more bits left before I can start using directly kernel header for newer tools. Moreover this particular parameter is only used for verbose mode which isn't used in default options. Regards, Santosh
From: Santosh Shilimkar <santosh.shilimkar@oracle.com> Date: Wed, 7 Dec 2016 08:44:04 -0800 > On 12/7/2016 7:55 AM, David Miller wrote: >> From: Santosh Shilimkar <santosh.shilimkar@oracle.com> >> Date: Tue, 6 Dec 2016 20:01:56 -0800 >> >> What level of compatability exists here? If we run an old tool on a >> new >> kernel, or a new tool on an old kernel, does it work properly? >> > Tools repo carries a copy of the header and thats how the old tool and > new tools have been running with older/newer kernels. There are few > more > bits left before I can start using directly kernel header for newer > tools. > > Moreover this particular parameter is only used for verbose mode which > isn't used in default options. That doesn't really answer my question, I think. Are you saying that one is required to run old tools on old kernels, and new tools on new kernels, and that's how you have this setup in your repo? If so, that really isn't acceptable. Both old and new tools must work with all kernel versions.
On 12/7/2016 9:05 AM, David Miller wrote: > From: Santosh Shilimkar <santosh.shilimkar@oracle.com> > Date: Wed, 7 Dec 2016 08:44:04 -0800 > >> On 12/7/2016 7:55 AM, David Miller wrote: >>> From: Santosh Shilimkar <santosh.shilimkar@oracle.com> >>> Date: Tue, 6 Dec 2016 20:01:56 -0800 >>> >>> What level of compatability exists here? If we run an old tool on a >>> new >>> kernel, or a new tool on an old kernel, does it work properly? >>> >> Tools repo carries a copy of the header and thats how the old tool and >> new tools have been running with older/newer kernels. There are few >> more >> bits left before I can start using directly kernel header for newer >> tools. >> >> Moreover this particular parameter is only used for verbose mode which >> isn't used in default options. > > That doesn't really answer my question, I think. > Sorry for not being clear. > Are you saying that one is required to run old tools on old kernels, > and new tools on new kernels, and that's how you have this setup in > your repo? > No. > If so, that really isn't acceptable. Both old and new tools must work > with all kernel versions. > Older version of tools works on either kernel versions. Older tools don't parse this additional info since its copy of header not carrying some of these extra verbose fields. Newer/Updated tools which can parse this extra info in needs newer or an updated kernel which supports and populates these fields. As mentioned, this particular option used only in verbose mode so am ok to drop this change if its still a concern. Regards, Santosh
From: Santosh Shilimkar <santosh.shilimkar@oracle.com> Date: Wed, 7 Dec 2016 09:20:17 -0800 > Newer/Updated tools which can parse this extra info in needs newer > or an updated kernel which supports and populates these fields. > > As mentioned, this particular option used only in verbose mode so > am ok to drop this change if its still a concern. What does the newer tool do on an older kernel if it doesn't see the fields? Does it check the size of the structure given back to it, and conditionally handle the older vs. the newer layout? It must do this.
On 12/7/2016 9:36 AM, David Miller wrote: [...] > What does the newer tool do on an older kernel if it doesn't see > the fields? Does it check the size of the structure given back > to it, and conditionally handle the older vs. the newer layout? > > It must do this. > Right but the rds-tool doesn't handle it well and needs to be fixed before this or any additional change in rds-info structures. To handle the kernel struct size issues with its user copy, there is a provision to probe kernel struct len and then decide on layout buts its not used/implemented in tools. The parsing comment I made is also not completely accurate. For now, I will drop this patch and submit it once the tools code is fixed along with other used fields to handle both layouts. I should have checked the full compatibility matrix as you commented. Sorry for oversight. Thanks for your comments Dave. Regard, Santosh
diff --git a/include/uapi/linux/rds.h b/include/uapi/linux/rds.h index 3833113..410ae3c 100644 --- a/include/uapi/linux/rds.h +++ b/include/uapi/linux/rds.h @@ -183,6 +183,7 @@ struct rds_info_rdma_connection { uint32_t max_send_sge; uint32_t rdma_mr_max; uint32_t rdma_mr_size; + uint32_t cache_allocs; }; /* RDS message Receive Path Latency points */ diff --git a/net/rds/ib.c b/net/rds/ib.c index 8d70884..b5e2699 100644 --- a/net/rds/ib.c +++ b/net/rds/ib.c @@ -313,6 +313,7 @@ static int rds_ib_conn_info_visitor(struct rds_connection *conn, iinfo->max_send_wr = ic->i_send_ring.w_nr; iinfo->max_recv_wr = ic->i_recv_ring.w_nr; iinfo->max_send_sge = rds_ibdev->max_sge; + iinfo->cache_allocs = atomic_read(&ic->i_cache_allocs); rds_ib_get_mr_info(rds_ibdev, iinfo); } return 1;
rds-tools already support it. Signed-off-by: Santosh Shilimkar <santosh.shilimkar@oracle.com> --- include/uapi/linux/rds.h | 1 + net/rds/ib.c | 1 + 2 files changed, 2 insertions(+)