Re: [lustre-devel] [PATCH 00/20] staging: lustre: convert to rhashtable

[Simmons, James A.]

>>> libcfs in lustre has a resizeable hashtable.
>>> Linux already has a resizeable hashtable, rhashtable, which is better
>>> is most metrics. See https://lwn.net/Articles/751374/ in a few days
>>> for an introduction to rhashtable.
>>
>> Thansk for starting this work. I was think about cleaning the libcfs
>> hash but your port to rhashtables is way better. How did you gather
>> metrics to see that rhashtable was better than libcfs hash?
>
>Code inspection and reputation.  It is hard to beat inlined lockless
>code for lookups.  And rhashtable is heavily used in the network
>subsystem and they are very focused on latency there.  I'm not sure that
>insertion is as fast as it can be (I have some thoughts on that) but I'm
>sure lookup will be better.
>I haven't done any performance testing myself, only correctness.

Sorry this email never reached my infradead account so I'm replying
from my work account.  I was just wondering if numbers were gathered.
I'm curious how well this would scale on some HPC cluster. In any case
I can do a comparison with and without the patches on one of my test
clusters and share the numbers with you using real world work loads.

>>> This series converts lustre to use rhashtable.  This affects several
>>> different tables, and each is different is various ways.
>>>
>>> There are two outstanding issues.  One is that a bug in rhashtable
>>> means that we cannot enable auto-shrinking in one of the tables.  That
>>> is documented as appropriate and should be fixed soon.
>>>
>>> The other is that rhashtable has an atomic_t which counts the elements
>>> in a hash table.  At least one table in lustre went to some trouble to
>>> avoid any table-wide atomics, so that could lead to a regression.
>>> I'm hoping that rhashtable can be enhanced with the option of a
>>> per-cpu counter, or similar.
>>>
>>
>> This doesn't sound quite ready to land just yet. This will have to do some
>> soak testing and a larger scope of test to make sure no new regressions
>> happen. Believe me I did work to make lustre work better on tickless
>> systems, which I'm preparing for the linux client, and small changes could
>> break things in interesting ways. I will port the rhashtable change to the
>> Intel developement branch and get people more familar with the hash code
>> to look at it.
>
>Whether it is "ready" or not probably depends on perspective and
>priorities.  As I see it, getting lustre cleaned up and out of staging
>is a fairly high priority, and it will require a lot of code change.
>It is inevitable that regressions will slip in (some already have) and
>it is important to keep testing (the test suite is of great benefit, but
>is only part of the story of course).  But to test the code, it needs to
>land.  Testing the code in Intel's devel branch and then porting it
>across doesn't really prove much.  For testing to be meaningful, it
>needs to be tested in a branch that up-to-date with mainline and on
>track to be merged into mainline.
>
>I have no particular desire to rush this in, but I don't see any
>particular benefit in delaying it either.
>
>I guess I see staging as implicitly a 'devel' branch.  You seem to be
>treating it a bit like a 'stable' branch - is that right?

So two years ago no one would touch the linux Lustre client due to it
being so broken. Then after about a year of work the client got into 
sort of working state. Even to the point actual sites are using it. 
It still is  broken and guess who gets notified of the brokenness :-)  
The good news is that people do actually test it but if it regress to much
we will lose our testing audience. Sadly its a chicken and egg problem. Yes I
want to see it leave staging but I like the Lustre client to be in good working
order. If it leaves staging still broken and no one uses it then their is not
much point. 

So to understand why I work with the Intel development branch and linux 
kernel version I need to explain my test setup.  So I test at different levels.

At level one I have my generic x86 nodes and x86 server back ends. Pretty
vanilla and the scale is pretty small. 3 servers nodes and a couple of clients.
In the environment I can easily test the upstream client. This is not VMs but
real hardware and real storage back end.

Besides x86 client nodes for level one testing I also have Power9 ppc nodes.
Also its possible for me to test the upstream client directly. In case you want 
to
know nope lustre doesn't work out of the box for Power9. The IB stack needs
fixing and we need to handle 64K pages at the LNet layer. I have work that
resolves those issues. The fixes need to be pushed upstream.

Next I have an ARM client cluster to test with which runs a newer kernel that 
is "offical".  When we first got the cluster I stomped all over it but 
discovered
I had to share it and the other parties involved were not to happy with my 
experimenting. The ARM system had the same issues as the Power9 so
now it works. Once the upstream client is in 

Re: [lustre-devel] [PATCH 00/20] staging: lustre: convert to rhashtable

[Simmons, James A.]

>>> libcfs in lustre has a resizeable hashtable.
>>> Linux already has a resizeable hashtable, rhashtable, which is better
>>> is most metrics. See https://lwn.net/Articles/751374/ in a few days
>>> for an introduction to rhashtable.
>>
>> Thansk for starting this work. I was think about cleaning the libcfs
>> hash but your port to rhashtables is way better. How did you gather
>> metrics to see that rhashtable was better than libcfs hash?
>
>Code inspection and reputation.  It is hard to beat inlined lockless
>code for lookups.  And rhashtable is heavily used in the network
>subsystem and they are very focused on latency there.  I'm not sure that
>insertion is as fast as it can be (I have some thoughts on that) but I'm
>sure lookup will be better.
>I haven't done any performance testing myself, only correctness.

Sorry this email never reached my infradead account so I'm replying
from my work account.  I was just wondering if numbers were gathered.
I'm curious how well this would scale on some HPC cluster. In any case
I can do a comparison with and without the patches on one of my test
clusters and share the numbers with you using real world work loads.

>>> This series converts lustre to use rhashtable.  This affects several
>>> different tables, and each is different is various ways.
>>>
>>> There are two outstanding issues.  One is that a bug in rhashtable
>>> means that we cannot enable auto-shrinking in one of the tables.  That
>>> is documented as appropriate and should be fixed soon.
>>>
>>> The other is that rhashtable has an atomic_t which counts the elements
>>> in a hash table.  At least one table in lustre went to some trouble to
>>> avoid any table-wide atomics, so that could lead to a regression.
>>> I'm hoping that rhashtable can be enhanced with the option of a
>>> per-cpu counter, or similar.
>>>
>>
>> This doesn't sound quite ready to land just yet. This will have to do some
>> soak testing and a larger scope of test to make sure no new regressions
>> happen. Believe me I did work to make lustre work better on tickless
>> systems, which I'm preparing for the linux client, and small changes could
>> break things in interesting ways. I will port the rhashtable change to the
>> Intel developement branch and get people more familar with the hash code
>> to look at it.
>
>Whether it is "ready" or not probably depends on perspective and
>priorities.  As I see it, getting lustre cleaned up and out of staging
>is a fairly high priority, and it will require a lot of code change.
>It is inevitable that regressions will slip in (some already have) and
>it is important to keep testing (the test suite is of great benefit, but
>is only part of the story of course).  But to test the code, it needs to
>land.  Testing the code in Intel's devel branch and then porting it
>across doesn't really prove much.  For testing to be meaningful, it
>needs to be tested in a branch that up-to-date with mainline and on
>track to be merged into mainline.
>
>I have no particular desire to rush this in, but I don't see any
>particular benefit in delaying it either.
>
>I guess I see staging as implicitly a 'devel' branch.  You seem to be
>treating it a bit like a 'stable' branch - is that right?

So two years ago no one would touch the linux Lustre client due to it
being so broken. Then after about a year of work the client got into 
sort of working state. Even to the point actual sites are using it. 
It still is  broken and guess who gets notified of the brokenness :-)  
The good news is that people do actually test it but if it regress to much
we will lose our testing audience. Sadly its a chicken and egg problem. Yes I
want to see it leave staging but I like the Lustre client to be in good working
order. If it leaves staging still broken and no one uses it then their is not
much point. 

So to understand why I work with the Intel development branch and linux 
kernel version I need to explain my test setup.  So I test at different levels.

At level one I have my generic x86 nodes and x86 server back ends. Pretty
vanilla and the scale is pretty small. 3 servers nodes and a couple of clients.
In the environment I can easily test the upstream client. This is not VMs but
real hardware and real storage back end.

Besides x86 client nodes for level one testing I also have Power9 ppc nodes.
Also its possible for me to test the upstream client directly. In case you want 
to
know nope lustre doesn't work out of the box for Power9. The IB stack needs
fixing and we need to handle 64K pages at the LNet layer. I have work that
resolves those issues. The fixes need to be pushed upstream.

Next I have an ARM client cluster to test with which runs a newer kernel that 
is "offical".  When we first got the cluster I stomped all over it but 
discovered
I had to share it and the other parties involved were not to happy with my 
experimenting. The ARM system had the same issues as the Power9 so
now it works. Once the upstream client is in 

RE: Staging: lustre: Make lustre_profile_list static

[Simmons, James A.]

>Variable lustre_profile_list is only used inside obd_config.c,
>better make it static
>
>Signed-off-by: Iban Rodriguez 

Acked-by: James Simmons 

>---
> drivers/staging/lustre/lustre/obdclass/obd_config.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/lustre/lustre/obdclass/obd_config.c 
b/drivers/staging/lustre/lustre/obdclass/obd_config.c
index 5395e994deab..8d484633b83a 100644
--- a/drivers/staging/lustre/lustre/obdclass/obd_config.c
+++ b/drivers/staging/lustre/lustre/obdclass/obd_config.c
@@ -606,7 +606,7 @@ static int class_del_conn(struct obd_device *obd, struct 
lustre_cfg *lcfg)
return rc;
 }
 
-LIST_HEAD(lustre_profile_list);
+static LIST_HEAD(lustre_profile_list);
 
 struct lustre_profile *class_get_profile(const char *prof)
 {
-- 
1.9.1

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

RE: Staging: lustre: Make lustre_profile_list static

[Simmons, James A.]

>Variable lustre_profile_list is only used inside obd_config.c,
>better make it static
>
>Signed-off-by: Iban Rodriguez 

Acked-by: James Simmons 

>---
> drivers/staging/lustre/lustre/obdclass/obd_config.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/lustre/lustre/obdclass/obd_config.c 
b/drivers/staging/lustre/lustre/obdclass/obd_config.c
index 5395e994deab..8d484633b83a 100644
--- a/drivers/staging/lustre/lustre/obdclass/obd_config.c
+++ b/drivers/staging/lustre/lustre/obdclass/obd_config.c
@@ -606,7 +606,7 @@ static int class_del_conn(struct obd_device *obd, struct 
lustre_cfg *lcfg)
return rc;
 }
 
-LIST_HEAD(lustre_profile_list);
+static LIST_HEAD(lustre_profile_list);
 
 struct lustre_profile *class_get_profile(const char *prof)
 {
-- 
1.9.1

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

RE: [lustre-devel] [RFC PATCH 0/3] staging: lustre: detypedef

[Simmons, James A.]

>Question about removing lustre typedefs.
>
>Various bits of lustre code use a mix of struct foo and foo_t.
>
>When would be an appropriate time to submit patches similar to
>below that individually remove various typedefs from lustre code?
>
>These are pretty trivial to produce and verify so there's no
>particular hurry to do them now but applying them will require
>resync points for active and actually useful developers.

Actually could you hold off for the LNet core and LND drivers these
changes. I have plans to push a few more LNet patches soon. I have
been just waiting for everyone to figure out how to deal with the 
latest changes to the infinband layer first. So the plan is to push
FMR support for the ko2iblnd driver. Also we have additional work
too handle setting the size of the DMA pools for o2iblnd but that
patch touches some of the core LNet code as well. Once those are
landed we can look at removing most of the typedefs. When its
time for the typedef to be cleaned up lets do just the structs first.
There are a few typedefs like lnet_nid_t I like to keep or if it has to
be changed turn it into a struct then. Things like lnet_nid_t act like
a cookie handle.

Now the best place to do this cleanup right now is LNet selftest. No
new code is planned for landing. We have lots of typedefs to remove
and I was planning to do that cleanup but if you want to do it just CC
me, jsimm...@infradead.org, so I can test the changes.

RE: [lustre-devel] [RFC PATCH 0/3] staging: lustre: detypedef

[Simmons, James A.]

>Question about removing lustre typedefs.
>
>Various bits of lustre code use a mix of struct foo and foo_t.
>
>When would be an appropriate time to submit patches similar to
>below that individually remove various typedefs from lustre code?
>
>These are pretty trivial to produce and verify so there's no
>particular hurry to do them now but applying them will require
>resync points for active and actually useful developers.

Actually could you hold off for the LNet core and LND drivers these
changes. I have plans to push a few more LNet patches soon. I have
been just waiting for everyone to figure out how to deal with the 
latest changes to the infinband layer first. So the plan is to push
FMR support for the ko2iblnd driver. Also we have additional work
too handle setting the size of the DMA pools for o2iblnd but that
patch touches some of the core LNet code as well. Once those are
landed we can look at removing most of the typedefs. When its
time for the typedef to be cleaned up lets do just the structs first.
There are a few typedefs like lnet_nid_t I like to keep or if it has to
be changed turn it into a struct then. Things like lnet_nid_t act like
a cookie handle.

Now the best place to do this cleanup right now is LNet selftest. No
new code is planned for landing. We have lots of typedefs to remove
and I was planning to do that cleanup but if you want to do it just CC
me, jsimm...@infradead.org, so I can test the changes.

RE: [lustre-devel] [PATCH 2/6] staging: lustre: libcfs: move memory_pressure functions to libcfs_prim.h

[Simmons, James A.]

>On Thu, Mar 31, 2016 at 10:18:36AM -0400, James Simmons wrote:
>> Long ago libcfs_prim.h was used for userland code which is why
>> memory_pressure_*() handling is in both libcfs_prim.h and
>> linux-mem.h headers. So lets just move the memory_pressure_*()
>> to libcfs_prim.h.
>> 
>> Signed-off-by: James Simmons 
>> ntel-bug-id: https://jira.hpdd.intel.com/browse/LU-6245
>
>"ntel-bug-id:"?  :)

Thanks for fixing that.

RE: [lustre-devel] [PATCH 2/6] staging: lustre: libcfs: move memory_pressure functions to libcfs_prim.h

[Simmons, James A.]

>On Thu, Mar 31, 2016 at 10:18:36AM -0400, James Simmons wrote:
>> Long ago libcfs_prim.h was used for userland code which is why
>> memory_pressure_*() handling is in both libcfs_prim.h and
>> linux-mem.h headers. So lets just move the memory_pressure_*()
>> to libcfs_prim.h.
>> 
>> Signed-off-by: James Simmons 
>> ntel-bug-id: https://jira.hpdd.intel.com/browse/LU-6245
>
>"ntel-bug-id:"?  :)

Thanks for fixing that.

RE: [lustre-devel] [PATCH] Revert "Staging: lustre: o2iblnd: Use sizeof type *pointer instead of sizeof type."

[Simmons, James A.]

>> > > so the right code should be:
>> > > 
>> > > sizeof(**net->ibn_tx_ps);
>> > > and the same for sizeof(**net->ibn_fmr_ps)
>> > That's a mess, isn't there some other way to fix this up to be more
>> > "obvious"?
>> This must have been encountered in the past. How was it handle in those
>> other cases?
>
>I fail to see why it's a mess.  It's just **
>and someone making a mistake.

I have no trouble with **.  If we revert it someone else will come along and
do the same mistake so I think we are stuck with the change to **.

>Removing the "typedef struct" uses from lustre
>would probably make a lot of this clearer though.

That along with a few hundred more patches heading Greg's way :-)

RE: [lustre-devel] [PATCH] Revert "Staging: lustre: o2iblnd: Use sizeof type *pointer instead of sizeof type."

[Simmons, James A.]

>> > > so the right code should be:
>> > > 
>> > > sizeof(**net->ibn_tx_ps);
>> > > and the same for sizeof(**net->ibn_fmr_ps)
>> > That's a mess, isn't there some other way to fix this up to be more
>> > "obvious"?
>> This must have been encountered in the past. How was it handle in those
>> other cases?
>
>I fail to see why it's a mess.  It's just **
>and someone making a mistake.

I have no trouble with **.  If we revert it someone else will come along and
do the same mistake so I think we are stuck with the change to **.

>Removing the "typedef struct" uses from lustre
>would probably make a lot of this clearer though.

That along with a few hundred more patches heading Greg's way :-)

RE: [lustre-devel] [PATCH] Revert "Staging: lustre: o2iblnd: Use sizeof type *pointer instead of sizeof type."

[Simmons, James A.]

>On Wed, Mar 23, 2016 at 05:39:36AM +, Dilger, Andreas wrote:
>> On 2016/03/22, 19:49, "lustre-devel on behalf of Greg Kroah-Hartman"
>> > gre...@linuxfoundation.org> wrote:
>> 
>> >On Tue, Mar 22, 2016 at 06:21:04PM -0400, James Simmons wrote:
>> >> Latest testing fails when using ko2iblnd. It was tracked down
>> >> to commit 4671a026616df26000f7d8ad2f2ea4b6de79263c.
>> >> 
>> >> This reverts commit 4671a026616df26000f7d8ad2f2ea4b6de79263c.
>> >> ---
>> >>  .../staging/lustre/lnet/klnds/o2iblnd/o2iblnd.c|4 ++--
>> >>  1 files changed, 2 insertions(+), 2 deletions(-)
>> >> 
>> >> diff --git a/drivers/staging/lustre/lnet/klnds/o2iblnd/o2iblnd.c
>> >>b/drivers/staging/lustre/lnet/klnds/o2iblnd/o2iblnd.c
>> >> index 89f9390..0d32e65 100644
>> >> --- a/drivers/staging/lustre/lnet/klnds/o2iblnd/o2iblnd.c
>> >> +++ b/drivers/staging/lustre/lnet/klnds/o2iblnd/o2iblnd.c
>> >> @@ -1968,7 +1968,7 @@ static int kiblnd_net_init_pools(kib_net_t *net,
>> >>__u32 *cpts, int ncpts)
>> >>*/
>> >>  
>> >>   net->ibn_fmr_ps = cfs_percpt_alloc(lnet_cpt_table(),
>> >> -sizeof(*net->ibn_fmr_ps));
>> >> +sizeof(kib_fmr_poolset_t));
>> >
>> >Ok, why is this revert needed?  Please give me a big huge comment about
>> >why this is not the same size of the variable being assigned to it,
>> >otherwise someone else is going to come along and make the exact same
>> >change again.
>> >
>> >>   if (!net->ibn_fmr_ps) {
>> >>   CERROR("Failed to allocate FMR pool array\n");
>> >>   rc = -ENOMEM;
>> >> @@ -1992,7 +1992,7 @@ static int kiblnd_net_init_pools(kib_net_t *net,
>> >>__u32 *cpts, int ncpts)
>> >>  
>> >>   create_tx_pool:
>> >>   net->ibn_tx_ps = cfs_percpt_alloc(lnet_cpt_table(),
>> >> -   sizeof(*net->ibn_tx_ps));
>> >> +   sizeof(kib_tx_poolset_t));
>> >
>> >Same here, why is this code wrong?
>> 
>> Looks like the declarations are:
>> 
>> kib_tx_poolset_t **ibn_tx_ps;/* tx pool-set */
>>  kib_fmr_poolset_t   **ibn_fmr_ps;   /* fmr pool-set */
>> 
>> 
>> 
>> so the right code should be:
>> 
>> sizeof(**net->ibn_tx_ps);
>> 
>> 
>> and the same for sizeof(**net->ibn_fmr_ps)
>
>That's a mess, isn't there some other way to fix this up to be more
>"obvious"?

This must have been encountered in the past. How was it handle in those
other cases?

RE: [lustre-devel] [PATCH] Revert "Staging: lustre: o2iblnd: Use sizeof type *pointer instead of sizeof type."

[Simmons, James A.]

>On Wed, Mar 23, 2016 at 05:39:36AM +, Dilger, Andreas wrote:
>> On 2016/03/22, 19:49, "lustre-devel on behalf of Greg Kroah-Hartman"
>> > gre...@linuxfoundation.org> wrote:
>> 
>> >On Tue, Mar 22, 2016 at 06:21:04PM -0400, James Simmons wrote:
>> >> Latest testing fails when using ko2iblnd. It was tracked down
>> >> to commit 4671a026616df26000f7d8ad2f2ea4b6de79263c.
>> >> 
>> >> This reverts commit 4671a026616df26000f7d8ad2f2ea4b6de79263c.
>> >> ---
>> >>  .../staging/lustre/lnet/klnds/o2iblnd/o2iblnd.c|4 ++--
>> >>  1 files changed, 2 insertions(+), 2 deletions(-)
>> >> 
>> >> diff --git a/drivers/staging/lustre/lnet/klnds/o2iblnd/o2iblnd.c
>> >>b/drivers/staging/lustre/lnet/klnds/o2iblnd/o2iblnd.c
>> >> index 89f9390..0d32e65 100644
>> >> --- a/drivers/staging/lustre/lnet/klnds/o2iblnd/o2iblnd.c
>> >> +++ b/drivers/staging/lustre/lnet/klnds/o2iblnd/o2iblnd.c
>> >> @@ -1968,7 +1968,7 @@ static int kiblnd_net_init_pools(kib_net_t *net,
>> >>__u32 *cpts, int ncpts)
>> >>*/
>> >>  
>> >>   net->ibn_fmr_ps = cfs_percpt_alloc(lnet_cpt_table(),
>> >> -sizeof(*net->ibn_fmr_ps));
>> >> +sizeof(kib_fmr_poolset_t));
>> >
>> >Ok, why is this revert needed?  Please give me a big huge comment about
>> >why this is not the same size of the variable being assigned to it,
>> >otherwise someone else is going to come along and make the exact same
>> >change again.
>> >
>> >>   if (!net->ibn_fmr_ps) {
>> >>   CERROR("Failed to allocate FMR pool array\n");
>> >>   rc = -ENOMEM;
>> >> @@ -1992,7 +1992,7 @@ static int kiblnd_net_init_pools(kib_net_t *net,
>> >>__u32 *cpts, int ncpts)
>> >>  
>> >>   create_tx_pool:
>> >>   net->ibn_tx_ps = cfs_percpt_alloc(lnet_cpt_table(),
>> >> -   sizeof(*net->ibn_tx_ps));
>> >> +   sizeof(kib_tx_poolset_t));
>> >
>> >Same here, why is this code wrong?
>> 
>> Looks like the declarations are:
>> 
>> kib_tx_poolset_t **ibn_tx_ps;/* tx pool-set */
>>  kib_fmr_poolset_t   **ibn_fmr_ps;   /* fmr pool-set */
>> 
>> 
>> 
>> so the right code should be:
>> 
>> sizeof(**net->ibn_tx_ps);
>> 
>> 
>> and the same for sizeof(**net->ibn_fmr_ps)
>
>That's a mess, isn't there some other way to fix this up to be more
>"obvious"?

This must have been encountered in the past. How was it handle in those
other cases?

RE: [PATCH] staging: lustre: really make lustre dependent on LNet

[Simmons, James A.]

>A patch intended to add a dependency on LNET for lustre didn't
>actually do that and instead allowed configurations that contain
>lustre with lnet but without IPv4 support that subsequently
>fail to link:
>
>warning: (LUSTRE_FS) selects LNET which has unmet direct dependencies (STAGING 
>&& INET && m && MODULES)
>ERROR: "kernel_sendmsg" [drivers/staging/lustre/lnet/lnet/lnet.ko] undefined!
>ERROR: "sock_create_lite" [drivers/staging/lustre/lnet/lnet/lnet.ko] undefined!
>ERROR: "sock_release" [drivers/staging/lustre/lnet/lnet/lnet.ko] undefined!
>ERROR: "release_sock" [drivers/staging/lustre/lnet/klnds/socklnd/ksocklnd.ko] 
>undefined!
>ERROR: "kernel_sendmsg" 
>[drivers/staging/lustre/lnet/klnds/socklnd/ksocklnd.ko] undefined!
>ERROR: "tcp_sendpage" [drivers/staging/lustre/lnet/klnds/socklnd/ksocklnd.ko] 
>undefined!
>
>This adds the one-line change that was evidently missing from the
>commit, doing what was intended there to have a correct set of dependencies.
>
>Signed-off-by: Arnd Bergmann 
>Fixes: b08bb6bb5af5 ("staging: lustre: make lustre dependent on LNet")

Thanks for fixing that. I was surprised that select didn't manage the 
configuration automatically.

Acked-by: James Simmons 

>---
> drivers/staging/lustre/lustre/Kconfig | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
>diff --git a/drivers/staging/lustre/lustre/Kconfig 
>b/drivers/staging/lustre/lustre/Kconfig
>index a09b51ce8265..8ac7cd4d6fdb 100644
>--- a/drivers/staging/lustre/lustre/Kconfig
>+++ b/drivers/staging/lustre/lustre/Kconfig
>@@ -1,7 +1,7 @@
> config LUSTRE_FS
>   tristate "Lustre file system client support"
>   depends on m && !MIPS && !XTENSA && !SUPERH
>-  select LNET
>+  depends on LNET
>   select CRYPTO
>   select CRYPTO_CRC32
>   select CRYPTO_CRC32_PCLMUL if X86
>-- 
>2.7.0

RE: [PATCH] staging: lustre: really make lustre dependent on LNet

[Simmons, James A.]

>A patch intended to add a dependency on LNET for lustre didn't
>actually do that and instead allowed configurations that contain
>lustre with lnet but without IPv4 support that subsequently
>fail to link:
>
>warning: (LUSTRE_FS) selects LNET which has unmet direct dependencies (STAGING 
>&& INET && m && MODULES)
>ERROR: "kernel_sendmsg" [drivers/staging/lustre/lnet/lnet/lnet.ko] undefined!
>ERROR: "sock_create_lite" [drivers/staging/lustre/lnet/lnet/lnet.ko] undefined!
>ERROR: "sock_release" [drivers/staging/lustre/lnet/lnet/lnet.ko] undefined!
>ERROR: "release_sock" [drivers/staging/lustre/lnet/klnds/socklnd/ksocklnd.ko] 
>undefined!
>ERROR: "kernel_sendmsg" 
>[drivers/staging/lustre/lnet/klnds/socklnd/ksocklnd.ko] undefined!
>ERROR: "tcp_sendpage" [drivers/staging/lustre/lnet/klnds/socklnd/ksocklnd.ko] 
>undefined!
>
>This adds the one-line change that was evidently missing from the
>commit, doing what was intended there to have a correct set of dependencies.
>
>Signed-off-by: Arnd Bergmann 
>Fixes: b08bb6bb5af5 ("staging: lustre: make lustre dependent on LNet")

Thanks for fixing that. I was surprised that select didn't manage the 
configuration automatically.

Acked-by: James Simmons 

>---
> drivers/staging/lustre/lustre/Kconfig | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
>diff --git a/drivers/staging/lustre/lustre/Kconfig 
>b/drivers/staging/lustre/lustre/Kconfig
>index a09b51ce8265..8ac7cd4d6fdb 100644
>--- a/drivers/staging/lustre/lustre/Kconfig
>+++ b/drivers/staging/lustre/lustre/Kconfig
>@@ -1,7 +1,7 @@
> config LUSTRE_FS
>   tristate "Lustre file system client support"
>   depends on m && !MIPS && !XTENSA && !SUPERH
>-  select LNET
>+  depends on LNET
>   select CRYPTO
>   select CRYPTO_CRC32
>   select CRYPTO_CRC32_PCLMUL if X86
>-- 
>2.7.0

RE: [lustre-devel] [PATCH 00/10] Last batch of fixes for LNet

[Simmons, James A.]

>On Fri, Mar 04, 2016 at 09:09:40PM -0500, James Simmons wrote:
>> This batch merges the remaining LNet patches from the OpenSFS
>> branch for the upstream client. Once merged the LNet code
>> will be up to date with the latest production code. Only style
>> issues are remaining. Still future patches being developed
>> for LNet will be landed to the upstream client as soon as they
>> are ready after extensive testing.
>
>Please fix up the build issue, and figure out what went wrong with the
>__user patch that you sent and resend this series after reworking them.

I had a discussion with Oleg about the __user patch. It  appears that is
a bug in the production branch so that patch can be dropped.  As for the
build issues this has been a know issue for a awhile but nobody has
gotten around to fixing all the 32 bit issues. I guess it is time to fix that 
up.
I will send out new patches later after I'm doing testing them.

RE: [lustre-devel] [PATCH 00/10] Last batch of fixes for LNet

[Simmons, James A.]

>On Fri, Mar 04, 2016 at 09:09:40PM -0500, James Simmons wrote:
>> This batch merges the remaining LNet patches from the OpenSFS
>> branch for the upstream client. Once merged the LNet code
>> will be up to date with the latest production code. Only style
>> issues are remaining. Still future patches being developed
>> for LNet will be landed to the upstream client as soon as they
>> are ready after extensive testing.
>
>Please fix up the build issue, and figure out what went wrong with the
>__user patch that you sent and resend this series after reworking them.

I had a discussion with Oleg about the __user patch. It  appears that is
a bug in the production branch so that patch can be dropped.  As for the
build issues this has been a know issue for a awhile but nobody has
gotten around to fixing all the 32 bit issues. I guess it is time to fix that 
up.
I will send out new patches later after I'm doing testing them.

RE: [lustre-devel] [PATCH 00/27] Third batch of LNet fixes

[Simmons, James A.]

>On Wed, Mar 02, 2016 at 05:01:43PM -0500, James Simmons wrote:
>> This patch set merges all the fixes for the klnd drivers, socklnd and
>> o2iblnd, to what is currently used in production environments. Several
>> more fixes for the LNet core are also included with this patch set.
>
>I've applied the first 20, I never got patch 21 in the series, so please
>rebase and resend the remaining ones, properly numbered :)
>
>thanks,

Okay. I will send the remaining patches as a new patch series.

RE: [lustre-devel] [PATCH 00/27] Third batch of LNet fixes

[Simmons, James A.]

>On Wed, Mar 02, 2016 at 05:01:43PM -0500, James Simmons wrote:
>> This patch set merges all the fixes for the klnd drivers, socklnd and
>> o2iblnd, to what is currently used in production environments. Several
>> more fixes for the LNet core are also included with this patch set.
>
>I've applied the first 20, I never got patch 21 in the series, so please
>rebase and resend the remaining ones, properly numbered :)
>
>thanks,

Okay. I will send the remaining patches as a new patch series.

RE: [lustre-devel] [PATCH] staging: lustre: Support different ko2iblnd configs between systems

[Simmons, James A.]

>On Wed, Mar 02, 2016 at 05:02:04PM -0500, James Simmons wrote:
>> From: Jeremy Filizetti 
>> 
>> This patch adds suppoort for ko2iblnd to have different values for
>> peer_credits and map_on_demand between systems.
>
>Your subject has no number for this patch, is it really patch 21 in the
>series?

Yes it is really patch 21. The patch needed to be updated at the last
minute before I sent it out. Sorry about that mistake.

RE: [lustre-devel] [PATCH] staging: lustre: Support different ko2iblnd configs between systems

[Simmons, James A.]

>On Wed, Mar 02, 2016 at 05:02:04PM -0500, James Simmons wrote:
>> From: Jeremy Filizetti 
>> 
>> This patch adds suppoort for ko2iblnd to have different values for
>> peer_credits and map_on_demand between systems.
>
>Your subject has no number for this patch, is it really patch 21 in the
>series?

Yes it is really patch 21. The patch needed to be updated at the last
minute before I sent it out. Sorry about that mistake.

RE: [lustre-devel] [PATCH v2 0/6] staging: lustre: update modinfo data

[Simmons, James A.]

>On Fri, Feb 26, 2016 at 06:11:07AM +, Drokin, Oleg wrote:
>> 
>> On Feb 26, 2016, at 1:03 AM, Greg Kroah-Hartman wrote:
>> 
>> > On Thu, Feb 25, 2016 at 08:07:06PM -0500, James Simmons wrote:
>> >> The module information for Lustre is stale or in some cases
>> >> completely missing. This collection of patches brings the
>> >> modinfo up to date as well as filling in any missing information.
>> >> This patch set has been redone to rebase it on Oleg's latest
>> >> patch set to avoid collisons in merging.
>> >> 
>> >> Andreas Dilger (4):
>> >>  staging: lustre: add missing MODULE_AUTHOR for LNet selftest module
>> >>  staging: lustre: update the MODULE_DESCRIPTION for all lustre modules
>> >>  staging: lustre: make module_init/exit naming consistent
>> >>  staging: lustre: update comment for lnet_lib_init/exit
>> >> 
>> >> James Simmons (2):
>> >>  staging: lustre: move module info to end of libcfs module.c file
>> >>  staging: lustre: update the MODULE_VERSION for all lustre modules
>> > 
>> > What changed to need a v2 of this series?
>> > 
>> > Please ALWAYS say what the difference is, don't expect us to "just
>> > know".
>> > 
>> > Please send a v3 of this, describing the changes, in the correct format,
>> > in each patch.  You know better than this…
>> 
>> I think it says above that the rebase was done on top of my patchset
>> to resolve the conflict that arose?
>
>Where?  Ugh, yeah, kind of, but where is the big v2: marking or some
>such thing like is required?  It's very easy to miss this (as I did.)
>Please make it easy for maintainers, not hard on them...

Sorry, in the future I will add a ChangeLog section for newer versions of 
patches.
The good news is the patches from Oleg's that conflicted landed already which
means the this patch set is ready to land.

RE: [lustre-devel] [PATCH v2 0/6] staging: lustre: update modinfo data

[Simmons, James A.]

>On Fri, Feb 26, 2016 at 06:11:07AM +, Drokin, Oleg wrote:
>> 
>> On Feb 26, 2016, at 1:03 AM, Greg Kroah-Hartman wrote:
>> 
>> > On Thu, Feb 25, 2016 at 08:07:06PM -0500, James Simmons wrote:
>> >> The module information for Lustre is stale or in some cases
>> >> completely missing. This collection of patches brings the
>> >> modinfo up to date as well as filling in any missing information.
>> >> This patch set has been redone to rebase it on Oleg's latest
>> >> patch set to avoid collisons in merging.
>> >> 
>> >> Andreas Dilger (4):
>> >>  staging: lustre: add missing MODULE_AUTHOR for LNet selftest module
>> >>  staging: lustre: update the MODULE_DESCRIPTION for all lustre modules
>> >>  staging: lustre: make module_init/exit naming consistent
>> >>  staging: lustre: update comment for lnet_lib_init/exit
>> >> 
>> >> James Simmons (2):
>> >>  staging: lustre: move module info to end of libcfs module.c file
>> >>  staging: lustre: update the MODULE_VERSION for all lustre modules
>> > 
>> > What changed to need a v2 of this series?
>> > 
>> > Please ALWAYS say what the difference is, don't expect us to "just
>> > know".
>> > 
>> > Please send a v3 of this, describing the changes, in the correct format,
>> > in each patch.  You know better than this…
>> 
>> I think it says above that the rebase was done on top of my patchset
>> to resolve the conflict that arose?
>
>Where?  Ugh, yeah, kind of, but where is the big v2: marking or some
>such thing like is required?  It's very easy to miss this (as I did.)
>Please make it easy for maintainers, not hard on them...

Sorry, in the future I will add a ChangeLog section for newer versions of 
patches.
The good news is the patches from Oleg's that conflicted landed already which
means the this patch set is ready to land.

RE: [lustre-devel] [PATCH 5/7] staging:lustre: simplify libcfs_psdev_[open|release]

[Simmons, James A.]

>> diff --git a/drivers/staging/lustre/lustre/libcfs/linux/linux-module.c 
>> b/drivers/staging/lustre/lustre/libcfs/linux/linux-module.c
>> index 33f6036..64f0fbf 100644
>> --- a/drivers/staging/lustre/lustre/libcfs/linux/linux-module.c
>> +++ b/drivers/staging/lustre/lustre/libcfs/linux/linux-module.c
>> @@ -98,30 +98,22 @@ int libcfs_ioctl_popdata(void *arg, void *data, int size)
>>  static int
>>  libcfs_psdev_open(struct inode *inode, struct file *file)
>>  {
>> -intrc = 0;
>> -
>>  if (!inode)
>>  return -EINVAL;
>> -if (libcfs_psdev_ops.p_open != NULL)
>> -rc = libcfs_psdev_ops.p_open(0, NULL);
>> -else
>> -return -EPERM;
>> -return rc;
>> +
>> +try_module_get(THIS_MODULE);
>
>Note, code like this is racy and incorrect and never needed, please fix
>this up properly (hint, set the module in the file operations.)
>
>Again, if you ever see code with that line, it is incorrect.

So simple

static struct file_operations libcfs_fops = {
.module  = THIS_MODULE,
.unlocked_ioctl = libcfs_psdev_ioctl,
};

With the open and release deleted should do the trick then.

RE: [lustre-devel] [PATCH 06/11] staging: lustre: add missing spaces for LNet layer reported by checkpatch.pl

[Simmons, James A.]

>On Fri, 2016-02-12 at 12:06 -0500, James Simmons wrote:
>> Add missing spaces in the code reported by checkpatch.pl.
>[]
>> diff --git a/drivers/staging/lustre/include/linux/lnet/lib-types.h 
>> b/drivers/staging/lustre/include/linux/lnet/lib-types.h
>[]
>> @@ -112,7 +112,7 @@ typedef struct lnet_libhandle {
>>  } lnet_libhandle_t;
>>  
>>  #define lh_entry(ptr, type, member) \
>> -((type *)((char *)(ptr)-(char *)(&((type *)0)->member)))
>> +((type *)((char *)(ptr) - (char *)(&((type *)0)->member)))
>
>This could use offsetof(type, member)

Will send a later patch to cover this.

RE: [lustre-devel] [PATCH 06/11] staging: lustre: add missing spaces for LNet layer reported by checkpatch.pl

[Simmons, James A.]

>On Fri, 2016-02-12 at 12:06 -0500, James Simmons wrote:
>> Add missing spaces in the code reported by checkpatch.pl.
>[]
>> diff --git a/drivers/staging/lustre/include/linux/lnet/lib-types.h 
>> b/drivers/staging/lustre/include/linux/lnet/lib-types.h
>[]
>> @@ -112,7 +112,7 @@ typedef struct lnet_libhandle {
>>  } lnet_libhandle_t;
>>  
>>  #define lh_entry(ptr, type, member) \
>> -((type *)((char *)(ptr)-(char *)(&((type *)0)->member)))
>> +((type *)((char *)(ptr) - (char *)(&((type *)0)->member)))
>
>This could use offsetof(type, member)

Will send a later patch to cover this.

RE: [lustre-devel] [PATCH 5/7] staging:lustre: simplify libcfs_psdev_[open|release]

[Simmons, James A.]

>> diff --git a/drivers/staging/lustre/lustre/libcfs/linux/linux-module.c 
>> b/drivers/staging/lustre/lustre/libcfs/linux/linux-module.c
>> index 33f6036..64f0fbf 100644
>> --- a/drivers/staging/lustre/lustre/libcfs/linux/linux-module.c
>> +++ b/drivers/staging/lustre/lustre/libcfs/linux/linux-module.c
>> @@ -98,30 +98,22 @@ int libcfs_ioctl_popdata(void *arg, void *data, int size)
>>  static int
>>  libcfs_psdev_open(struct inode *inode, struct file *file)
>>  {
>> -intrc = 0;
>> -
>>  if (!inode)
>>  return -EINVAL;
>> -if (libcfs_psdev_ops.p_open != NULL)
>> -rc = libcfs_psdev_ops.p_open(0, NULL);
>> -else
>> -return -EPERM;
>> -return rc;
>> +
>> +try_module_get(THIS_MODULE);
>
>Note, code like this is racy and incorrect and never needed, please fix
>this up properly (hint, set the module in the file operations.)
>
>Again, if you ever see code with that line, it is incorrect.

So simple

static struct file_operations libcfs_fops = {
.module  = THIS_MODULE,
.unlocked_ioctl = libcfs_psdev_ioctl,
};

With the open and release deleted should do the trick then.

RE: [lustre-devel] [PATCH] staging: lustre/lustre/libcfs: Fix type mismatch reported by sparse

[Simmons, James A.]

>>>2. Is it OK to hardcode the appropriate gfp_t flags for the
>>>IOC_LIBCFS_MEMHOG, as the userspace
>>>seems to be taking the decision about the page allocation
>>>zone/strategy, is this what is intended?
>>
>> The memhog functionality is used to introduce memory pressure on a client
>> or server during operation to test error handling as well as memory
>> allocation deadlocks (e.g. GFP_KERNEL used where GFP_NOFS should be used).
>> There are other ways to do this in the kernel today, so all of the memhog
>> code could just be deleted I think.
>>
>> This looks like kportal_memhog_alloc(), kportal_memhog_free(),
>> IOC_LIBCFS_MEMHOG, and struct libcfs_device_userstate could be removed.
>>
>>
>> Cheers, Andreas
>>
>Thanks Andreas, I will send out a separate patch with the cleanup as
>you suggested.

I missed this email. I just sent the cleanup patches a bit ago.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [lustre-devel] [PATCH] staging: lustre/lustre/libcfs: Fix type mismatch reported by sparse

[Simmons, James A.]

>> On Wed, Dec 09, 2015 at 10:38:13PM +0530, Niranjan Dighe wrote:
>>> The third argument to function kportal_memhog_alloc is expected to
>>> be gfp_t whereas the actual argument was unsigned int. Fix this by
>>> explicitly typecasting to gfp_t
>>>
>>> Signed-off-by: Niranjan Dighe 
>>> ---
>>>  drivers/staging/lustre/lustre/libcfs/module.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/staging/lustre/lustre/libcfs/module.c 
>>> b/drivers/staging/lustre/lustre/libcfs/module.c
>>> index 96d9d46..9c79f6e 100644
>>> --- a/drivers/staging/lustre/lustre/libcfs/module.c
>>> +++ b/drivers/staging/lustre/lustre/libcfs/module.c
>>> @@ -268,7 +268,7 @@ static int libcfs_ioctl_int(struct cfs_psdev_file 
>>> *pfile, unsigned long cmd,
>>>   /* XXX The ioc_flags is not GFP flags now, need to be 
>>> fixed */
>>>   err = kportal_memhog_alloc(pfile->private_data,
>>>  data->ioc_count,
>>> -data->ioc_flags);
>>> + (__force gfp_t)data->ioc_flags);
>>
>> No, please fix the type to be correct properly, like the comment says
>> needs to be done.
>>
>> thanks,
>>
>> greg k-h
>
>Hello Greg,
>
>I could see that the ioc_flags member of the struct libcfs_ioctl_data
>is used as gfp_t only in the
>case of the ioctl IOC_LIBCFS_MEMHOG. I can think of following ways to
>correct it -

IOC_LIBCFS_MEMHOG will be going away. Since this keeps coming up I will prepare 
some patches.
Especially now that out tools no longer uses these obsolete ioctls.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [lustre-devel] [PATCH] staging: lustre/lustre/libcfs: Fix type mismatch reported by sparse

[Simmons, James A.]

>> On Wed, Dec 09, 2015 at 10:38:13PM +0530, Niranjan Dighe wrote:
>>> The third argument to function kportal_memhog_alloc is expected to
>>> be gfp_t whereas the actual argument was unsigned int. Fix this by
>>> explicitly typecasting to gfp_t
>>>
>>> Signed-off-by: Niranjan Dighe 
>>> ---
>>>  drivers/staging/lustre/lustre/libcfs/module.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/staging/lustre/lustre/libcfs/module.c 
>>> b/drivers/staging/lustre/lustre/libcfs/module.c
>>> index 96d9d46..9c79f6e 100644
>>> --- a/drivers/staging/lustre/lustre/libcfs/module.c
>>> +++ b/drivers/staging/lustre/lustre/libcfs/module.c
>>> @@ -268,7 +268,7 @@ static int libcfs_ioctl_int(struct cfs_psdev_file 
>>> *pfile, unsigned long cmd,
>>>   /* XXX The ioc_flags is not GFP flags now, need to be 
>>> fixed */
>>>   err = kportal_memhog_alloc(pfile->private_data,
>>>  data->ioc_count,
>>> -data->ioc_flags);
>>> + (__force gfp_t)data->ioc_flags);
>>
>> No, please fix the type to be correct properly, like the comment says
>> needs to be done.
>>
>> thanks,
>>
>> greg k-h
>
>Hello Greg,
>
>I could see that the ioc_flags member of the struct libcfs_ioctl_data
>is used as gfp_t only in the
>case of the ioctl IOC_LIBCFS_MEMHOG. I can think of following ways to
>correct it -

IOC_LIBCFS_MEMHOG will be going away. Since this keeps coming up I will prepare 
some patches.
Especially now that out tools no longer uses these obsolete ioctls.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [lustre-devel] [PATCH] staging: lustre/lustre/libcfs: Fix type mismatch reported by sparse

[Simmons, James A.]

>>>2. Is it OK to hardcode the appropriate gfp_t flags for the
>>>IOC_LIBCFS_MEMHOG, as the userspace
>>>seems to be taking the decision about the page allocation
>>>zone/strategy, is this what is intended?
>>
>> The memhog functionality is used to introduce memory pressure on a client
>> or server during operation to test error handling as well as memory
>> allocation deadlocks (e.g. GFP_KERNEL used where GFP_NOFS should be used).
>> There are other ways to do this in the kernel today, so all of the memhog
>> code could just be deleted I think.
>>
>> This looks like kportal_memhog_alloc(), kportal_memhog_free(),
>> IOC_LIBCFS_MEMHOG, and struct libcfs_device_userstate could be removed.
>>
>>
>> Cheers, Andreas
>>
>Thanks Andreas, I will send out a separate patch with the cleanup as
>you suggested.

I missed this email. I just sent the cleanup patches a bit ago.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [lustre-devel] [PATCH] staging: lustre: Remove unused memhog functionality

[Simmons, James A.]

>On 2015/12/23, 14:40, "Simmons, James A."  wrote:
>
>>>From: Niranjan Dighe 
>>>
>>>Remove IOC_LIBCFS_MEMHOG ioctl functionality as it is no longer needed
>>>thereby
>>>making functions like - kportal_memhog_alloc(), kportal_memhog_free()
>>>and type -
>>>struct libcfs_device_userstate unused.
>>
>>Actually so much more can be cleaned up. This work is also being done at
>>
>>http://review.whamcloud.com/#/c/17492.
>>
>>I will update that patch and post it here as well.
>
>James, part of that patch is for the userspace tools, which isn't
>applicable here.  Also, it probably makes sense to delete the panic injection 
>as a
>separate patch, so I think this patch is good as-is for removing memhog.

After looking at the code I agree. At first I thought it would be a simple 
cleanup expansion
of this patch but I see a lot more cleanups coming after this. This work is the 
first step to
removing the cfs_psdev_* abstraction. I will submit the cleanups soon. For now 
this patch
can be merged.

Acked-by: James Simmons 

>
>
>Signed-off-by: Niranjan Dighe 
>---
> .../lustre/include/linux/libcfs/libcfs_private.h   |5 -
> .../lustre/lustre/libcfs/linux/linux-module.c  |   14 +-
> drivers/staging/lustre/lustre/libcfs/module.c  |  139
>
> 3 files changed, 2 insertions(+), 156 deletions(-)
>
>diff --git a/drivers/staging/lustre/include/linux/libcfs/libcfs_private.h
>b/drivers/staging/lustre/include/linux/libcfs/libcfs_private.h
>index d6273e1..e044d6f 100644
>--- a/drivers/staging/lustre/include/linux/libcfs/libcfs_private.h
>+++ b/drivers/staging/lustre/include/linux/libcfs/libcfs_private.h
>@@ -391,11 +391,6 @@ int cfs_percpt_atomic_summary(atomic_t **refs);
>  * Support for temporary event tracing with minimal Heisenberg effect.
>  * 
>*/
> 
>-struct libcfs_device_userstate {
>-  intldu_memhog_pages;
>-  struct page   *ldu_memhog_root_page;
>-};
>-
> #define MKSTR(ptr) ((ptr)) ? (ptr) : ""
> 
> static inline int cfs_size_round4(int val)
>diff --git a/drivers/staging/lustre/lustre/libcfs/linux/linux-module.c
>b/drivers/staging/lustre/lustre/libcfs/linux/linux-module.c
>index 70a99cf0..eccfe8bd 100644
>--- a/drivers/staging/lustre/lustre/libcfs/linux/linux-module.c
>+++ b/drivers/staging/lustre/lustre/libcfs/linux/linux-module.c
>@@ -98,14 +98,12 @@ int libcfs_ioctl_popdata(void *arg, void *data, int
>size)
> static int
> libcfs_psdev_open(struct inode *inode, struct file *file)
> {
>-  struct libcfs_device_userstate **pdu = NULL;
>   intrc = 0;
> 
>   if (!inode)
>   return -EINVAL;
>-  pdu = (struct libcfs_device_userstate **)>private_data;
>   if (libcfs_psdev_ops.p_open != NULL)
>-  rc = libcfs_psdev_ops.p_open(0, (void *)pdu);
>+  rc = libcfs_psdev_ops.p_open(0, NULL);
>   else
>   return -EPERM;
>   return rc;
>@@ -115,14 +113,12 @@ libcfs_psdev_open(struct inode *inode, struct file
>*file)
> static int
> libcfs_psdev_release(struct inode *inode, struct file *file)
> {
>-  struct libcfs_device_userstate *pdu;
>   intrc = 0;
> 
>   if (!inode)
>   return -EINVAL;
>-  pdu = file->private_data;
>   if (libcfs_psdev_ops.p_close != NULL)
>-  rc = libcfs_psdev_ops.p_close(0, (void *)pdu);
>+  rc = libcfs_psdev_ops.p_close(0, NULL);
>   else
>   rc = -EPERM;
>   return rc;
>@@ -152,14 +148,8 @@ static long libcfs_ioctl(struct file *file,
>   return -EPERM;
>   panic("debugctl-invoked panic");
>   return 0;
>-  case IOC_LIBCFS_MEMHOG:
>-  if (!capable(CFS_CAP_SYS_ADMIN))
>-  return -EPERM;
>-  /* go thought */
>   }
> 
>-  pfile.off = 0;
>-  pfile.private_data = file->private_data;
>   if (libcfs_psdev_ops.p_ioctl != NULL)
>   rc = libcfs_psdev_ops.p_ioctl(, cmd, (void *)arg);
>   else
>diff --git a/drivers/staging/lustre/lustre/libcfs/module.c
>b/drivers/staging/lustre/lustre/libcfs/module.c
>index 329d78c..0067e53 100644
>--- a/drivers/staging/lustre/lustre/libcfs/module.c
>+++ b/drivers/staging/lustre/lustre/libcfs/module.c
>@@ -68,142 +68,16 @@ MODULE_LICENSE("GPL");
> 
> static struct dentry *lnet_debugfs_root;
> 
>-static void kportal_memhog_free(struct libcfs_device_userstate *ldu)
>-{
>-  struct page **level0p = >ldu_memhog_root_page;
>-  struct page *

RE: [lustre-devel] [PATCH] staging: lustre: Remove unused memhog functionality

[Simmons, James A.]

>From: Niranjan Dighe 
>
>Remove IOC_LIBCFS_MEMHOG ioctl functionality as it is no longer needed thereby
>making functions like - kportal_memhog_alloc(), kportal_memhog_free() and type 
>-
>struct libcfs_device_userstate unused.

Actually so much more can be cleaned up. This work is also being done at 

http://review.whamcloud.com/#/c/17492.

I will update that patch and post it here as well.


Signed-off-by: Niranjan Dighe 
---
 .../lustre/include/linux/libcfs/libcfs_private.h   |5 -
 .../lustre/lustre/libcfs/linux/linux-module.c  |   14 +-
 drivers/staging/lustre/lustre/libcfs/module.c  |  139 
 3 files changed, 2 insertions(+), 156 deletions(-)

diff --git a/drivers/staging/lustre/include/linux/libcfs/libcfs_private.h 
b/drivers/staging/lustre/include/linux/libcfs/libcfs_private.h
index d6273e1..e044d6f 100644
--- a/drivers/staging/lustre/include/linux/libcfs/libcfs_private.h
+++ b/drivers/staging/lustre/include/linux/libcfs/libcfs_private.h
@@ -391,11 +391,6 @@ int cfs_percpt_atomic_summary(atomic_t **refs);
  * Support for temporary event tracing with minimal Heisenberg effect.
  *  */
 
-struct libcfs_device_userstate {
-   intldu_memhog_pages;
-   struct page   *ldu_memhog_root_page;
-};
-
 #define MKSTR(ptr) ((ptr)) ? (ptr) : ""
 
 static inline int cfs_size_round4(int val)
diff --git a/drivers/staging/lustre/lustre/libcfs/linux/linux-module.c 
b/drivers/staging/lustre/lustre/libcfs/linux/linux-module.c
index 70a99cf0..eccfe8bd 100644
--- a/drivers/staging/lustre/lustre/libcfs/linux/linux-module.c
+++ b/drivers/staging/lustre/lustre/libcfs/linux/linux-module.c
@@ -98,14 +98,12 @@ int libcfs_ioctl_popdata(void *arg, void *data, int size)
 static int
 libcfs_psdev_open(struct inode *inode, struct file *file)
 {
-   struct libcfs_device_userstate **pdu = NULL;
intrc = 0;
 
if (!inode)
return -EINVAL;
-   pdu = (struct libcfs_device_userstate **)>private_data;
if (libcfs_psdev_ops.p_open != NULL)
-   rc = libcfs_psdev_ops.p_open(0, (void *)pdu);
+   rc = libcfs_psdev_ops.p_open(0, NULL);
else
return -EPERM;
return rc;
@@ -115,14 +113,12 @@ libcfs_psdev_open(struct inode *inode, struct file *file)
 static int
 libcfs_psdev_release(struct inode *inode, struct file *file)
 {
-   struct libcfs_device_userstate *pdu;
intrc = 0;
 
if (!inode)
return -EINVAL;
-   pdu = file->private_data;
if (libcfs_psdev_ops.p_close != NULL)
-   rc = libcfs_psdev_ops.p_close(0, (void *)pdu);
+   rc = libcfs_psdev_ops.p_close(0, NULL);
else
rc = -EPERM;
return rc;
@@ -152,14 +148,8 @@ static long libcfs_ioctl(struct file *file,
return -EPERM;
panic("debugctl-invoked panic");
return 0;
-   case IOC_LIBCFS_MEMHOG:
-   if (!capable(CFS_CAP_SYS_ADMIN))
-   return -EPERM;
-   /* go thought */
}
 
-   pfile.off = 0;
-   pfile.private_data = file->private_data;
if (libcfs_psdev_ops.p_ioctl != NULL)
rc = libcfs_psdev_ops.p_ioctl(, cmd, (void *)arg);
else
diff --git a/drivers/staging/lustre/lustre/libcfs/module.c 
b/drivers/staging/lustre/lustre/libcfs/module.c
index 329d78c..0067e53 100644
--- a/drivers/staging/lustre/lustre/libcfs/module.c
+++ b/drivers/staging/lustre/lustre/libcfs/module.c
@@ -68,142 +68,16 @@ MODULE_LICENSE("GPL");
 
 static struct dentry *lnet_debugfs_root;
 
-static void kportal_memhog_free(struct libcfs_device_userstate *ldu)
-{
-   struct page **level0p = >ldu_memhog_root_page;
-   struct page **level1p;
-   struct page **level2p;
-   intcount1;
-   intcount2;
-
-   if (*level0p != NULL) {
-
-   level1p = (struct page **)page_address(*level0p);
-   count1 = 0;
-
-   while (count1 < PAGE_CACHE_SIZE/sizeof(struct page *) &&
-  *level1p != NULL) {
-
-   level2p = (struct page **)page_address(*level1p);
-   count2 = 0;
-
-   while (count2 < PAGE_CACHE_SIZE/sizeof(struct page *) &&
-  *level2p != NULL) {
-
-   __free_page(*level2p);
-   ldu->ldu_memhog_pages--;
-   level2p++;
-   count2++;
-   }
-
-   __free_page(*level1p);
-   ldu->ldu_memhog_pages--;
-   level1p++;
-   count1++;
-   }
-
-   __free_page(*level0p);
-   ldu->ldu_memhog_pages--;
-
-   *level0p = NULL;
-   }
-
-   

RE: [lustre-devel] [PATCH] staging: lustre: Remove unused memhog functionality

[Simmons, James A.]

>On 2015/12/23, 14:40, "Simmons, James A." <simmon...@ornl.gov> wrote:
>
>>>From: Niranjan Dighe <ndi...@visteon.com>
>>>
>>>Remove IOC_LIBCFS_MEMHOG ioctl functionality as it is no longer needed
>>>thereby
>>>making functions like - kportal_memhog_alloc(), kportal_memhog_free()
>>>and type -
>>>struct libcfs_device_userstate unused.
>>
>>Actually so much more can be cleaned up. This work is also being done at
>>
>>http://review.whamcloud.com/#/c/17492.
>>
>>I will update that patch and post it here as well.
>
>James, part of that patch is for the userspace tools, which isn't
>applicable here.  Also, it probably makes sense to delete the panic injection 
>as a
>separate patch, so I think this patch is good as-is for removing memhog.

After looking at the code I agree. At first I thought it would be a simple 
cleanup expansion
of this patch but I see a lot more cleanups coming after this. This work is the 
first step to
removing the cfs_psdev_* abstraction. I will submit the cleanups soon. For now 
this patch
can be merged.

Acked-by: James Simmons <jsimm...@infradead.org>

>
>
>Signed-off-by: Niranjan Dighe <ndi...@visteon.com>
>---
> .../lustre/include/linux/libcfs/libcfs_private.h   |5 -
> .../lustre/lustre/libcfs/linux/linux-module.c  |   14 +-
> drivers/staging/lustre/lustre/libcfs/module.c  |  139
>
> 3 files changed, 2 insertions(+), 156 deletions(-)
>
>diff --git a/drivers/staging/lustre/include/linux/libcfs/libcfs_private.h
>b/drivers/staging/lustre/include/linux/libcfs/libcfs_private.h
>index d6273e1..e044d6f 100644
>--- a/drivers/staging/lustre/include/linux/libcfs/libcfs_private.h
>+++ b/drivers/staging/lustre/include/linux/libcfs/libcfs_private.h
>@@ -391,11 +391,6 @@ int cfs_percpt_atomic_summary(atomic_t **refs);
>  * Support for temporary event tracing with minimal Heisenberg effect.
>  * 
>*/
> 
>-struct libcfs_device_userstate {
>-  intldu_memhog_pages;
>-  struct page   *ldu_memhog_root_page;
>-};
>-
> #define MKSTR(ptr) ((ptr)) ? (ptr) : ""
> 
> static inline int cfs_size_round4(int val)
>diff --git a/drivers/staging/lustre/lustre/libcfs/linux/linux-module.c
>b/drivers/staging/lustre/lustre/libcfs/linux/linux-module.c
>index 70a99cf0..eccfe8bd 100644
>--- a/drivers/staging/lustre/lustre/libcfs/linux/linux-module.c
>+++ b/drivers/staging/lustre/lustre/libcfs/linux/linux-module.c
>@@ -98,14 +98,12 @@ int libcfs_ioctl_popdata(void *arg, void *data, int
>size)
> static int
> libcfs_psdev_open(struct inode *inode, struct file *file)
> {
>-  struct libcfs_device_userstate **pdu = NULL;
>   intrc = 0;
> 
>   if (!inode)
>   return -EINVAL;
>-  pdu = (struct libcfs_device_userstate **)>private_data;
>   if (libcfs_psdev_ops.p_open != NULL)
>-  rc = libcfs_psdev_ops.p_open(0, (void *)pdu);
>+  rc = libcfs_psdev_ops.p_open(0, NULL);
>   else
>   return -EPERM;
>   return rc;
>@@ -115,14 +113,12 @@ libcfs_psdev_open(struct inode *inode, struct file
>*file)
> static int
> libcfs_psdev_release(struct inode *inode, struct file *file)
> {
>-  struct libcfs_device_userstate *pdu;
>   intrc = 0;
> 
>   if (!inode)
>   return -EINVAL;
>-  pdu = file->private_data;
>   if (libcfs_psdev_ops.p_close != NULL)
>-  rc = libcfs_psdev_ops.p_close(0, (void *)pdu);
>+  rc = libcfs_psdev_ops.p_close(0, NULL);
>   else
>   rc = -EPERM;
>   return rc;
>@@ -152,14 +148,8 @@ static long libcfs_ioctl(struct file *file,
>   return -EPERM;
>   panic("debugctl-invoked panic");
>   return 0;
>-  case IOC_LIBCFS_MEMHOG:
>-  if (!capable(CFS_CAP_SYS_ADMIN))
>-  return -EPERM;
>-  /* go thought */
>   }
> 
>-  pfile.off = 0;
>-  pfile.private_data = file->private_data;
>   if (libcfs_psdev_ops.p_ioctl != NULL)
>   rc = libcfs_psdev_ops.p_ioctl(, cmd, (void *)arg);
>   else
>diff --git a/drivers/staging/lustre/lustre/libcfs/module.c
>b/drivers/staging/lustre/lustre/libcfs/module.c
>index 329d78c..0067e53 100644
>--- a/drivers/staging/lustre/lustre/libcfs/module.c
>+++ b/drivers/staging/lustre/lustre/libcfs/module.c
>@@ -68,142 +68,16 @@ MODULE_LICENSE("GPL");
> 
> static struct dentry *lnet_debugfs_root;
> 
>-static void kportal_memhog_free(struct libcfs_device_usersta

RE: [lustre-devel] [PATCH] staging: lustre: Remove unused memhog functionality

[Simmons, James A.]

>From: Niranjan Dighe 
>
>Remove IOC_LIBCFS_MEMHOG ioctl functionality as it is no longer needed thereby
>making functions like - kportal_memhog_alloc(), kportal_memhog_free() and type 
>-
>struct libcfs_device_userstate unused.

Actually so much more can be cleaned up. This work is also being done at 

http://review.whamcloud.com/#/c/17492.

I will update that patch and post it here as well.


Signed-off-by: Niranjan Dighe 
---
 .../lustre/include/linux/libcfs/libcfs_private.h   |5 -
 .../lustre/lustre/libcfs/linux/linux-module.c  |   14 +-
 drivers/staging/lustre/lustre/libcfs/module.c  |  139 
 3 files changed, 2 insertions(+), 156 deletions(-)

diff --git a/drivers/staging/lustre/include/linux/libcfs/libcfs_private.h 
b/drivers/staging/lustre/include/linux/libcfs/libcfs_private.h
index d6273e1..e044d6f 100644
--- a/drivers/staging/lustre/include/linux/libcfs/libcfs_private.h
+++ b/drivers/staging/lustre/include/linux/libcfs/libcfs_private.h
@@ -391,11 +391,6 @@ int cfs_percpt_atomic_summary(atomic_t **refs);
  * Support for temporary event tracing with minimal Heisenberg effect.
  *  */
 
-struct libcfs_device_userstate {
-   intldu_memhog_pages;
-   struct page   *ldu_memhog_root_page;
-};
-
 #define MKSTR(ptr) ((ptr)) ? (ptr) : ""
 
 static inline int cfs_size_round4(int val)
diff --git a/drivers/staging/lustre/lustre/libcfs/linux/linux-module.c 
b/drivers/staging/lustre/lustre/libcfs/linux/linux-module.c
index 70a99cf0..eccfe8bd 100644
--- a/drivers/staging/lustre/lustre/libcfs/linux/linux-module.c
+++ b/drivers/staging/lustre/lustre/libcfs/linux/linux-module.c
@@ -98,14 +98,12 @@ int libcfs_ioctl_popdata(void *arg, void *data, int size)
 static int
 libcfs_psdev_open(struct inode *inode, struct file *file)
 {
-   struct libcfs_device_userstate **pdu = NULL;
intrc = 0;
 
if (!inode)
return -EINVAL;
-   pdu = (struct libcfs_device_userstate **)>private_data;
if (libcfs_psdev_ops.p_open != NULL)
-   rc = libcfs_psdev_ops.p_open(0, (void *)pdu);
+   rc = libcfs_psdev_ops.p_open(0, NULL);
else
return -EPERM;
return rc;
@@ -115,14 +113,12 @@ libcfs_psdev_open(struct inode *inode, struct file *file)
 static int
 libcfs_psdev_release(struct inode *inode, struct file *file)
 {
-   struct libcfs_device_userstate *pdu;
intrc = 0;
 
if (!inode)
return -EINVAL;
-   pdu = file->private_data;
if (libcfs_psdev_ops.p_close != NULL)
-   rc = libcfs_psdev_ops.p_close(0, (void *)pdu);
+   rc = libcfs_psdev_ops.p_close(0, NULL);
else
rc = -EPERM;
return rc;
@@ -152,14 +148,8 @@ static long libcfs_ioctl(struct file *file,
return -EPERM;
panic("debugctl-invoked panic");
return 0;
-   case IOC_LIBCFS_MEMHOG:
-   if (!capable(CFS_CAP_SYS_ADMIN))
-   return -EPERM;
-   /* go thought */
}
 
-   pfile.off = 0;
-   pfile.private_data = file->private_data;
if (libcfs_psdev_ops.p_ioctl != NULL)
rc = libcfs_psdev_ops.p_ioctl(, cmd, (void *)arg);
else
diff --git a/drivers/staging/lustre/lustre/libcfs/module.c 
b/drivers/staging/lustre/lustre/libcfs/module.c
index 329d78c..0067e53 100644
--- a/drivers/staging/lustre/lustre/libcfs/module.c
+++ b/drivers/staging/lustre/lustre/libcfs/module.c
@@ -68,142 +68,16 @@ MODULE_LICENSE("GPL");
 
 static struct dentry *lnet_debugfs_root;
 
-static void kportal_memhog_free(struct libcfs_device_userstate *ldu)
-{
-   struct page **level0p = >ldu_memhog_root_page;
-   struct page **level1p;
-   struct page **level2p;
-   intcount1;
-   intcount2;
-
-   if (*level0p != NULL) {
-
-   level1p = (struct page **)page_address(*level0p);
-   count1 = 0;
-
-   while (count1 < PAGE_CACHE_SIZE/sizeof(struct page *) &&
-  *level1p != NULL) {
-
-   level2p = (struct page **)page_address(*level1p);
-   count2 = 0;
-
-   while (count2 < PAGE_CACHE_SIZE/sizeof(struct page *) &&
-  *level2p != NULL) {
-
-   __free_page(*level2p);
-   ldu->ldu_memhog_pages--;
-   level2p++;
-   count2++;
-   }
-
-   __free_page(*level1p);
-   ldu->ldu_memhog_pages--;
-   level1p++;
-   count1++;
-   }
-
-   __free_page(*level0p);
-   ldu->ldu_memhog_pages--;
-
-   *level0p = 

RE: [PATCH 13/40] staging: lustre: Dynamic LNet Configuration (DLC) show command

[Simmons, James A.]

>On Tue, Dec 15, 2015 at 06:14:19PM +0000, Simmons, James A. wrote:
>>   
>> >>  struct libcfs_ioctl_hdr {
>> >>   __u32 ioc_len;
>> >> @@ -87,6 +88,13 @@ do {   \
>> >>   data.ioc_hdr.ioc_len = sizeof(data);\
>> >>  } while (0)
>> >>  
>> >> +#define LIBCFS_IOC_INIT_V2(data, hdr)\
>> >> +do { \
>> >> + memset(&(data), 0, sizeof(data));   \
>> >> + (data).hdr.ioc_version = LIBCFS_IOCTL_VERSION2; \
>> >> + (data).hdr.ioc_len = sizeof(data);  \
>> >> +} while (0)
>> >> +
>> >
>> >Do we really need this?
>> 
>> Would you be okay if this was a inline function? This is used by user
>> land and kernel space code.
>
>Then your code is broken, please never do that.

This brings up a good point. This header doesn't contain structures for 
userland so it is a uapi
type header.  Should such headers only contain data structures?

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH 13/40] staging: lustre: Dynamic LNet Configuration (DLC) show command

[Simmons, James A.]

>On Tue, Dec 15, 2015 at 06:14:19PM +0000, Simmons, James A. wrote:
>>   
>> >>  struct libcfs_ioctl_hdr {
>> >>   __u32 ioc_len;
>> >> @@ -87,6 +88,13 @@ do {   \
>> >>   data.ioc_hdr.ioc_len = sizeof(data);\
>> >>  } while (0)
>> >>  
>> >> +#define LIBCFS_IOC_INIT_V2(data, hdr)\
>> >> +do { \
>> >> + memset(&(data), 0, sizeof(data));   \
>> >> + (data).hdr.ioc_version = LIBCFS_IOCTL_VERSION2; \
>> >> + (data).hdr.ioc_len = sizeof(data);  \
>> >> +} while (0)
>> >> +
>> >
>> >Do we really need this?
>> 
>> Would you be okay if this was a inline function? This is used by user land 
>> and kernel space code.
>> 
>
>I try (not very hard) to sound like a broken record but this business of
>sharing code with userland is a pain in the butt.  It's not used in the
>kernel or in any patches you have sent.
>
>It would look better as an inline function though so I wouldn't have
>even noticed it.

I'm glad you noticed.  I just looked at the production source code and yep it 
is only used
in the userland tools code. I need to update our tools so they don't break. 
Then we can
remove these macros.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH 13/40] staging: lustre: Dynamic LNet Configuration (DLC) show command

[Simmons, James A.]

  
>>  struct libcfs_ioctl_hdr {
>>  __u32 ioc_len;
>> @@ -87,6 +88,13 @@ do {  \
>>  data.ioc_hdr.ioc_len = sizeof(data);\
>>  } while (0)
>>  
>> +#define LIBCFS_IOC_INIT_V2(data, hdr)   \
>> +do {\
>> +memset(&(data), 0, sizeof(data));   \
>> +(data).hdr.ioc_version = LIBCFS_IOCTL_VERSION2; \
>> +(data).hdr.ioc_len = sizeof(data);  \
>> +} while (0)
>> +
>
>Do we really need this?

Would you be okay if this was a inline function? This is used by user land and 
kernel space code.


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH 02/40] staging: lustre: fix 'NULL pointer dereference' errors for LNet

[Simmons, James A.]

>> diff --git a/drivers/staging/lustre/lnet/selftest/conctl.c 
>> b/drivers/staging/lustre/lnet/selftest/conctl.c
>> index 556c837..2ca7d0e 100644
>> --- a/drivers/staging/lustre/lnet/selftest/conctl.c
>> +++ b/drivers/staging/lustre/lnet/selftest/conctl.c
>> @@ -679,45 +679,46 @@ static int
>>  lst_stat_query_ioctl(lstio_stat_args_t *args)
>>  {
>>  int rc;
>> -char *name;
>> +char *name = NULL;
>>  
>>  /* TODO: not finished */
>>  if (args->lstio_sta_key != console_session.ses_key)
>>  return -EACCES;
>>  
>> -if (args->lstio_sta_resultp == NULL ||
>> -(args->lstio_sta_namep  == NULL &&
>> - args->lstio_sta_idsp   == NULL) ||
>> -args->lstio_sta_nmlen <= 0 ||
>> -args->lstio_sta_nmlen > LST_NAME_SIZE)
>> -return -EINVAL;
>> -
>> -if (args->lstio_sta_idsp != NULL &&
>> -args->lstio_sta_count <= 0)
>> +if (!args->lstio_sta_resultp)
>>  return -EINVAL;
>>  
>> -LIBCFS_ALLOC(name, args->lstio_sta_nmlen + 1);
>> -if (name == NULL)
>> -return -ENOMEM;
>> -
>> -if (copy_from_user(name, args->lstio_sta_namep,
>> -   args->lstio_sta_nmlen)) {
>> -LIBCFS_FREE(name, args->lstio_sta_nmlen + 1);
>> -return -EFAULT;
>> -}
>> +if (args->lstio_sta_idsp) {
>> +if (args->lstio_sta_count <= 0)
>> +return -EINVAL;
>>  
>> -if (args->lstio_sta_idsp == NULL) {
>> -rc = lstcon_group_stat(name, args->lstio_sta_timeout,
>> -   args->lstio_sta_resultp);
>> -} else {
>>  rc = lstcon_nodes_stat(args->lstio_sta_count,
>> args->lstio_sta_idsp,
>> args->lstio_sta_timeout,
>> args->lstio_sta_resultp);
>> -}
>> +} else if (args->lstio_sta_namep) {
>> +if (args->lstio_sta_nmlen <= 0 ||
>> +args->lstio_sta_nmlen > LST_NAME_SIZE)
>> +return -EINVAL;
>> +
>> +LIBCFS_ALLOC(name, args->lstio_sta_nmlen + 1);
>> +if (!name)
>> +return -ENOMEM;
>>  
>> -LIBCFS_FREE(name, args->lstio_sta_nmlen + 1);
>> +rc = copy_from_user(name, args->lstio_sta_namep,
>> +args->lstio_sta_nmlen);
>> +if (!rc)
>> +rc = lstcon_group_stat(name, args->lstio_sta_timeout,
>> +   args->lstio_sta_resultp);
>> +else
>> +rc = -EFAULT;
>>  
>> +} else {
>> +rc = -EINVAL;
>> +}
>> +
>> +if (name)
>> +LIBCFS_FREE(name, args->lstio_sta_nmlen + 1);
>
>There is no bug fix here.  This code was fine when it was merged into
>the kernel in 2013 so I have no idea how out of date the static checker
>warning is...  The new code doesn't do unnecessary allocations so that's
>good but "name" should be declared in the block where it is used instead
>of at the start of the function.  Btw, we assume that the user gives us
>a NUL terminated string for "name" so we should fix that bug as well.
>
>TODO: lustre: don't assume "name" is NUL terminated

Ugh. I see breakage everywhere in this code :-( Need to address.  I think we
should convert that to strcpy_to_user as well.


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH 21/40] staging: lustre: improve LNet clean up code and API

[Simmons, James A.]

>Actually we're going to have to redo so much code that it's not worth it
>for me to review the rest of these patches.  

Sorry I didn't get back to you sooner but I was on vacation.  Thanks for 
reviewing this work. Especially since this is the first major bug fixing merge
for the lustre client which means a lot of pain involved to iron out how to
do this. I have been pondering if pushing bug fixes before style cleanups
is the right thing to do. I pushed a bunch of bug fixes earlier and none got
merged which either means Greg is just backed up and hasn't the time to
merge them or  style issues are higher priority. Assuming these bug fixes are
in scope of the staging tree. Should I continue to push this work first?
Well either way I should update this patch series so it ready to merge at some
point.

>Please just look over everything again:
>
> BAD:  return -1;
>GOOD:  return -EINVAL;
>
> BAD:  failed0:
>GOOD:  free_something:
>
> BAD:  if (rc != 0)
>GOOD:  if (rc)
>
>Do one thing per patch.
>Do not introduce a bug and then fix it in a later patch.
>Check ioc_len more carefully.
>Don't make the code look ugly just to please checkpatch.pl.
>Do error handling not success handling.
>Try to avoid indenting a far to the right.

Okay. Will start to do the patch cleanup.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH 02/40] staging: lustre: fix 'NULL pointer dereference' errors for LNet

[Simmons, James A.]

>> diff --git a/drivers/staging/lustre/lnet/selftest/conctl.c 
>> b/drivers/staging/lustre/lnet/selftest/conctl.c
>> index 556c837..2ca7d0e 100644
>> --- a/drivers/staging/lustre/lnet/selftest/conctl.c
>> +++ b/drivers/staging/lustre/lnet/selftest/conctl.c
>> @@ -679,45 +679,46 @@ static int
>>  lst_stat_query_ioctl(lstio_stat_args_t *args)
>>  {
>>  int rc;
>> -char *name;
>> +char *name = NULL;
>>  
>>  /* TODO: not finished */
>>  if (args->lstio_sta_key != console_session.ses_key)
>>  return -EACCES;
>>  
>> -if (args->lstio_sta_resultp == NULL ||
>> -(args->lstio_sta_namep  == NULL &&
>> - args->lstio_sta_idsp   == NULL) ||
>> -args->lstio_sta_nmlen <= 0 ||
>> -args->lstio_sta_nmlen > LST_NAME_SIZE)
>> -return -EINVAL;
>> -
>> -if (args->lstio_sta_idsp != NULL &&
>> -args->lstio_sta_count <= 0)
>> +if (!args->lstio_sta_resultp)
>>  return -EINVAL;
>>  
>> -LIBCFS_ALLOC(name, args->lstio_sta_nmlen + 1);
>> -if (name == NULL)
>> -return -ENOMEM;
>> -
>> -if (copy_from_user(name, args->lstio_sta_namep,
>> -   args->lstio_sta_nmlen)) {
>> -LIBCFS_FREE(name, args->lstio_sta_nmlen + 1);
>> -return -EFAULT;
>> -}
>> +if (args->lstio_sta_idsp) {
>> +if (args->lstio_sta_count <= 0)
>> +return -EINVAL;
>>  
>> -if (args->lstio_sta_idsp == NULL) {
>> -rc = lstcon_group_stat(name, args->lstio_sta_timeout,
>> -   args->lstio_sta_resultp);
>> -} else {
>>  rc = lstcon_nodes_stat(args->lstio_sta_count,
>> args->lstio_sta_idsp,
>> args->lstio_sta_timeout,
>> args->lstio_sta_resultp);
>> -}
>> +} else if (args->lstio_sta_namep) {
>> +if (args->lstio_sta_nmlen <= 0 ||
>> +args->lstio_sta_nmlen > LST_NAME_SIZE)
>> +return -EINVAL;
>> +
>> +LIBCFS_ALLOC(name, args->lstio_sta_nmlen + 1);
>> +if (!name)
>> +return -ENOMEM;
>>  
>> -LIBCFS_FREE(name, args->lstio_sta_nmlen + 1);
>> +rc = copy_from_user(name, args->lstio_sta_namep,
>> +args->lstio_sta_nmlen);
>> +if (!rc)
>> +rc = lstcon_group_stat(name, args->lstio_sta_timeout,
>> +   args->lstio_sta_resultp);
>> +else
>> +rc = -EFAULT;
>>  
>> +} else {
>> +rc = -EINVAL;
>> +}
>> +
>> +if (name)
>> +LIBCFS_FREE(name, args->lstio_sta_nmlen + 1);
>
>There is no bug fix here.  This code was fine when it was merged into
>the kernel in 2013 so I have no idea how out of date the static checker
>warning is...  The new code doesn't do unnecessary allocations so that's
>good but "name" should be declared in the block where it is used instead
>of at the start of the function.  Btw, we assume that the user gives us
>a NUL terminated string for "name" so we should fix that bug as well.
>
>TODO: lustre: don't assume "name" is NUL terminated

Ugh. I see breakage everywhere in this code :-( Need to address.  I think we
should convert that to strcpy_to_user as well.


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH 13/40] staging: lustre: Dynamic LNet Configuration (DLC) show command

[Simmons, James A.]

  
>>  struct libcfs_ioctl_hdr {
>>  __u32 ioc_len;
>> @@ -87,6 +88,13 @@ do {  \
>>  data.ioc_hdr.ioc_len = sizeof(data);\
>>  } while (0)
>>  
>> +#define LIBCFS_IOC_INIT_V2(data, hdr)   \
>> +do {\
>> +memset(&(data), 0, sizeof(data));   \
>> +(data).hdr.ioc_version = LIBCFS_IOCTL_VERSION2; \
>> +(data).hdr.ioc_len = sizeof(data);  \
>> +} while (0)
>> +
>
>Do we really need this?

Would you be okay if this was a inline function? This is used by user land and 
kernel space code.


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH 21/40] staging: lustre: improve LNet clean up code and API

[Simmons, James A.]

>Actually we're going to have to redo so much code that it's not worth it
>for me to review the rest of these patches.  

Sorry I didn't get back to you sooner but I was on vacation.  Thanks for 
reviewing this work. Especially since this is the first major bug fixing merge
for the lustre client which means a lot of pain involved to iron out how to
do this. I have been pondering if pushing bug fixes before style cleanups
is the right thing to do. I pushed a bunch of bug fixes earlier and none got
merged which either means Greg is just backed up and hasn't the time to
merge them or  style issues are higher priority. Assuming these bug fixes are
in scope of the staging tree. Should I continue to push this work first?
Well either way I should update this patch series so it ready to merge at some
point.

>Please just look over everything again:
>
> BAD:  return -1;
>GOOD:  return -EINVAL;
>
> BAD:  failed0:
>GOOD:  free_something:
>
> BAD:  if (rc != 0)
>GOOD:  if (rc)
>
>Do one thing per patch.
>Do not introduce a bug and then fix it in a later patch.
>Check ioc_len more carefully.
>Don't make the code look ugly just to please checkpatch.pl.
>Do error handling not success handling.
>Try to avoid indenting a far to the right.

Okay. Will start to do the patch cleanup.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH 13/40] staging: lustre: Dynamic LNet Configuration (DLC) show command

[Simmons, James A.]

>On Tue, Dec 15, 2015 at 06:14:19PM +0000, Simmons, James A. wrote:
>>   
>> >>  struct libcfs_ioctl_hdr {
>> >>   __u32 ioc_len;
>> >> @@ -87,6 +88,13 @@ do {   \
>> >>   data.ioc_hdr.ioc_len = sizeof(data);\
>> >>  } while (0)
>> >>  
>> >> +#define LIBCFS_IOC_INIT_V2(data, hdr)\
>> >> +do { \
>> >> + memset(&(data), 0, sizeof(data));   \
>> >> + (data).hdr.ioc_version = LIBCFS_IOCTL_VERSION2; \
>> >> + (data).hdr.ioc_len = sizeof(data);  \
>> >> +} while (0)
>> >> +
>> >
>> >Do we really need this?
>> 
>> Would you be okay if this was a inline function? This is used by user
>> land and kernel space code.
>
>Then your code is broken, please never do that.

This brings up a good point. This header doesn't contain structures for 
userland so it is a uapi
type header.  Should such headers only contain data structures?

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH 13/40] staging: lustre: Dynamic LNet Configuration (DLC) show command

[Simmons, James A.]

>On Tue, Dec 15, 2015 at 06:14:19PM +0000, Simmons, James A. wrote:
>>   
>> >>  struct libcfs_ioctl_hdr {
>> >>   __u32 ioc_len;
>> >> @@ -87,6 +88,13 @@ do {   \
>> >>   data.ioc_hdr.ioc_len = sizeof(data);\
>> >>  } while (0)
>> >>  
>> >> +#define LIBCFS_IOC_INIT_V2(data, hdr)\
>> >> +do { \
>> >> + memset(&(data), 0, sizeof(data));   \
>> >> + (data).hdr.ioc_version = LIBCFS_IOCTL_VERSION2; \
>> >> + (data).hdr.ioc_len = sizeof(data);  \
>> >> +} while (0)
>> >> +
>> >
>> >Do we really need this?
>> 
>> Would you be okay if this was a inline function? This is used by user land 
>> and kernel space code.
>> 
>
>I try (not very hard) to sound like a broken record but this business of
>sharing code with userland is a pain in the butt.  It's not used in the
>kernel or in any patches you have sent.
>
>It would look better as an inline function though so I wouldn't have
>even noticed it.

I'm glad you noticed.  I just looked at the production source code and yep it 
is only used
in the userland tools code. I need to update our tools so they don't break. 
Then we can
remove these macros.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [lustre-devel] [PATCH] staging: lustre: Handle nodemask on UMP machines

[Simmons, James A.]

>On Sun, Nov 08, 2015 at 11:34:55AM -0500, James Simmons wrote:
>> For UMP and SMP machines the struct cfs_cpt_table are
>> defined differently. In the case handled by this patch
>> nodemask is defined as a integer for the UMP case and
>> as a pointer for the SMP case. This will cause a problem
>> for ost_setup which reads the nodemask directly. Instead
>> we create a UMP version of cfs_cpt_nodemask and use that
>> in ost_setup.
>> 
>> Signed-off-by: James Simmons 
>> Intel-bug-id: https://jira.hpdd.intel.com/browse/LU-4199
>> Reviewed-on: http://review.whamcloud.com/9219
>> Reviewed-by: Liang Zhen 
>> Reviewed-by: Li Xi 
>> Reviewed-by: Andreas Dilger 
>
>Signed-off-by: and Reviewed-by: tags entered two times. Once here.
 
It is one of those rare situations where two patches are need together.
It would be nice to be able to preserve the reviewers for each one.


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [lustre-devel] [PATCH] staging: lustre: export cfs_str2mask

[Simmons, James A.]

>> We need cfs_str2mask exported for our server code.
>> Even with the server code not available upstream
>> it would be nice to use the upstream code on Lustre
>> servers.
>> 
>> Signed-off-by: James Simmons 
>> ---
>> .../staging/lustre/lustre/libcfs/libcfs_string.c   |1 +
>> 1 files changed, 1 insertions(+), 0 deletions(-)
>> 
>> diff --git a/drivers/staging/lustre/lustre/libcfs/libcfs_string.c 
>> b/drivers/staging/lustre/lustre/libcfs/libcfs_string.c
>> index d40be53..05630f8 100644
>> --- a/drivers/staging/lustre/lustre/libcfs/libcfs_string.c
>> +++ b/drivers/staging/lustre/lustre/libcfs/libcfs_string.c
>> @@ -111,6 +111,7 @@ int cfs_str2mask(const char *str, const char 
>> *(*bit2str)(int bit),
>>  *oldmask = newmask;
>>  return 0;
>> }
>> +EXPORT_SYMBOL(cfs_str2mask);
>
>If this is the case of it being used out of tree, I suspect a comment here to 
>that effect would be
>useful, otherwise next person running a script to eliminate unused 
>EXPORT_SYMBOLs would kill it again.

I will send another patch with comments not to remove the new code.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [lustre-devel] next-20151101 - depmod issues with Lustre modules

[Simmons, James A.]

>Reproduced on mainline v4.3-9038-g27eb427bdc0960 with
>Arch Linux default config (attached):
>
>depmod: ERROR: Found 2 modules in dependency cycles!
>depmod: ERROR: Cycle detected: lnet -> libcfs -> lnet
>make: *** [_modinst_post] Error 1

I submitted a patch (staging: lustre: remove IOC_LIBCFS_PING_TEST ioctl) to fix 
this.
It should be merged soon.

RE: [PATCH 8/9] Staging: lustre: llite_lib: Remove wrapper function

[Simmons, James A.]

>On Mon, Nov 9, 2015 at 7:07 PM, Michał Kępień  wrote:
>>> Remove the function ll_finish_md_op_data() and replace all its calls
>>> with the standrd function ll_finish_md_op_data().
>>
>> I believe you meant to write "standard function kfree()".
>>
>
>Yes. I am so sorry. Should I be sending the whole series again?
>Thank you
>Shivani

Yes please redo the series. I saw Oleg's concern and I would recommend that
besides the conversion to kfree that you add comments about what is being 
deleted.
I.E

/* Free struct md_op_data  data*/
kfree(...)
N�r��yb�X��ǧv�^�)޺{.n�+{zX����ܨ}���Ơz�:+v���zZ+��+zf���h���~i���z��w���?�&�)ߢf��^jǫy�m��@A�a���
0��h���i

RE: [lustre-devel] next-20151101 - depmod issues with Lustre modules

[Simmons, James A.]

>Reproduced on mainline v4.3-9038-g27eb427bdc0960 with
>Arch Linux default config (attached):
>
>depmod: ERROR: Found 2 modules in dependency cycles!
>depmod: ERROR: Cycle detected: lnet -> libcfs -> lnet
>make: *** [_modinst_post] Error 1

I submitted a patch (staging: lustre: remove IOC_LIBCFS_PING_TEST ioctl) to fix 
this.
It should be merged soon.

RE: [PATCH 8/9] Staging: lustre: llite_lib: Remove wrapper function

[Simmons, James A.]

>On Mon, Nov 9, 2015 at 7:07 PM, Michał Kępień  wrote:
>>> Remove the function ll_finish_md_op_data() and replace all its calls
>>> with the standrd function ll_finish_md_op_data().
>>
>> I believe you meant to write "standard function kfree()".
>>
>
>Yes. I am so sorry. Should I be sending the whole series again?
>Thank you
>Shivani

Yes please redo the series. I saw Oleg's concern and I would recommend that
besides the conversion to kfree that you add comments about what is being 
deleted.
I.E

/* Free struct md_op_data  data*/
kfree(...)
N�r��yb�X��ǧv�^�)޺{.n�+{zX����ܨ}���Ơz�:+v���zZ+��+zf���h���~i���z��w���?�&�)ߢf��^jǫy�m��@A�a���
0��h���i

RE: [lustre-devel] [PATCH] staging: lustre: export cfs_str2mask

[Simmons, James A.]

>> We need cfs_str2mask exported for our server code.
>> Even with the server code not available upstream
>> it would be nice to use the upstream code on Lustre
>> servers.
>> 
>> Signed-off-by: James Simmons 
>> ---
>> .../staging/lustre/lustre/libcfs/libcfs_string.c   |1 +
>> 1 files changed, 1 insertions(+), 0 deletions(-)
>> 
>> diff --git a/drivers/staging/lustre/lustre/libcfs/libcfs_string.c 
>> b/drivers/staging/lustre/lustre/libcfs/libcfs_string.c
>> index d40be53..05630f8 100644
>> --- a/drivers/staging/lustre/lustre/libcfs/libcfs_string.c
>> +++ b/drivers/staging/lustre/lustre/libcfs/libcfs_string.c
>> @@ -111,6 +111,7 @@ int cfs_str2mask(const char *str, const char 
>> *(*bit2str)(int bit),
>>  *oldmask = newmask;
>>  return 0;
>> }
>> +EXPORT_SYMBOL(cfs_str2mask);
>
>If this is the case of it being used out of tree, I suspect a comment here to 
>that effect would be
>useful, otherwise next person running a script to eliminate unused 
>EXPORT_SYMBOLs would kill it again.

I will send another patch with comments not to remove the new code.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [lustre-devel] [PATCH] staging: lustre: Handle nodemask on UMP machines

[Simmons, James A.]

>On Sun, Nov 08, 2015 at 11:34:55AM -0500, James Simmons wrote:
>> For UMP and SMP machines the struct cfs_cpt_table are
>> defined differently. In the case handled by this patch
>> nodemask is defined as a integer for the UMP case and
>> as a pointer for the SMP case. This will cause a problem
>> for ost_setup which reads the nodemask directly. Instead
>> we create a UMP version of cfs_cpt_nodemask and use that
>> in ost_setup.
>> 
>> Signed-off-by: James Simmons 
>> Intel-bug-id: https://jira.hpdd.intel.com/browse/LU-4199
>> Reviewed-on: http://review.whamcloud.com/9219
>> Reviewed-by: Liang Zhen 
>> Reviewed-by: Li Xi 
>> Reviewed-by: Andreas Dilger 
>
>Signed-off-by: and Reviewed-by: tags entered two times. Once here.
 
It is one of those rare situations where two patches are need together.
It would be nice to be able to preserve the reviewers for each one.


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [lustre-devel] [PATCH 07/10] staging: lustre: Handle nodemask on UMP machines

[Simmons, James A.]

>All warnings (new ones prefixed by >>):
>
>   In file included from include/linux/bitops.h:36:0,
>from 
> drivers/staging/lustre/lustre/libcfs/../../include/linux/libcfs/linux/libcfs.h:44,
>from 
> drivers/staging/lustre/lustre/libcfs/../../include/linux/libcfs/libcfs.h:40,
>from drivers/staging/lustre/lustre/libcfs/libcfs_cpu.c:38:
>   drivers/staging/lustre/lustre/libcfs/libcfs_cpu.c: In function 
> 'cfs_cpt_table_alloc':
>>> arch/m68k/include/asm/bitops.h:64:5: warning: passing argument 2 of 
>>> 'bset_mem_set_bit' from incompatible pointer type
>bset_mem_set_bit(nr, vaddr) : \
>^
>>> drivers/staging/lustre/lustre/libcfs/libcfs_cpu.c:61:3: note: in expansion 
>>> of macro 'set_bit'
>  set_bit(0, >ctb_nodemask);

Yep and additional patch exist to fix this. Should I just push the fix for this 
or drop this patch and create
a new patch that is combo of both fixes.

BTW Greg this new batch of patches are order independent. Sorry for not 
pointing that out. The rest of the
patch appear to be okay.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [lustre-devel] [PATCH] staging: lustre: lnet: klnds: socklnd: Move extern declarations to header

[Simmons, James A.]

>From: lustre-devel [mailto:lustre-devel-boun...@lists.lustre.org] On Behalf Of 
>Amitoj Kaur Chawla
>Sent: Friday, November 06, 2015 9:57 AM
>To: oleg.dro...@intel.com; andreas.dil...@intel.com; 
>gre...@linuxfoundation.org; lustre-de...@lists.lustre.org; 
>de...@driverdev.osuosl.org; linux-kernel@vger.kernel.org
>Subject: [lustre-devel] [PATCH] staging: lustre: lnet: klnds: socklnd: Move 
>extern declarations to header
>
>This patch moves extern declarations in socklnd_lib.c to the respective 
>header file, 'socklnd.h'.
>
>This patch also removes extern keyword from function declarations
>since functions have the extern specifier by default.
>
>Signed-off-by: Amitoj Kaur Chawla 
>---
> drivers/staging/lustre/lnet/klnds/socklnd/socklnd.h | 3 +++
> drivers/staging/lustre/lnet/klnds/socklnd/socklnd_lib.c | 2 --
> 2 files changed, 3 insertions(+), 2 deletions(-)

Acked-by: James Simmons 

diff --git a/drivers/staging/lustre/lnet/klnds/socklnd/socklnd.h 
b/drivers/staging/lustre/lnet/klnds/socklnd/socklnd.h
index b349847..f4fa725 100644
--- a/drivers/staging/lustre/lnet/klnds/socklnd/socklnd.h
+++ b/drivers/staging/lustre/lnet/klnds/socklnd/socklnd.h
@@ -679,6 +679,9 @@ int ksocknal_lib_recv_kiov(ksock_conn_t *conn);
 int ksocknal_lib_get_conn_tunables(ksock_conn_t *conn, int *txmem,
   int *rxmem, int *nagle);
 
+void ksocknal_read_callback(ksock_conn_t *conn);
+void ksocknal_write_callback(ksock_conn_t *conn);
+
 int ksocknal_tunables_init(void);
 
 void ksocknal_lib_csum_tx(ksock_tx_t *tx);
diff --git a/drivers/staging/lustre/lnet/klnds/socklnd/socklnd_lib.c 
b/drivers/staging/lustre/lnet/klnds/socklnd/socklnd_lib.c
index 679785b..04a4653 100644
--- a/drivers/staging/lustre/lnet/klnds/socklnd/socklnd_lib.c
+++ b/drivers/staging/lustre/lnet/klnds/socklnd/socklnd_lib.c
@@ -580,8 +580,6 @@ ksocknal_lib_push_conn(ksock_conn_t *conn)
ksocknal_connsock_decref(conn);
 }
 
-extern void ksocknal_read_callback(ksock_conn_t *conn);
-extern void ksocknal_write_callback(ksock_conn_t *conn);
 /*
  * socket call back in Linux
  */
-- 
1.9.1

___
lustre-devel mailing list
lustre-de...@lists.lustre.org
http://lists.lustre.org/listinfo.cgi/lustre-devel-lustre.org

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH] Staging: lustre: dir: Remove wrapper function

[Simmons, James A.]

>From: devel [mailto:driverdev-devel-boun...@linuxdriverproject.org] On Behalf 
>Of Shivani Bhardwaj
>Sent: Friday, November 06, 2015 11:55 AM
>To: gre...@linuxfoundation.org
>Cc: oleg.dro...@intel.com; de...@driverdev.osuosl.org; 
>andreas.dil...@intel.com; linux-kernel@vger.kernel.org; 
>lustre-de...@lists.lustre.org
>Subject: [PATCH] Staging: lustre: dir: Remove wrapper function
>
>Remove the function ll_check_page() and replace all its calls with the
>function it wrapped.
>
>Signed-off-by: Shivani Bhardwaj 
>---
> drivers/staging/lustre/lustre/llite/dir.c | 9 ++---
> 1 file changed, 2 insertions(+), 7 deletions(-)

Acked-by: James Simmons 

diff --git a/drivers/staging/lustre/lustre/llite/dir.c 
b/drivers/staging/lustre/lustre/llite/dir.c
index 5c9502b..842e6d6 100644
--- a/drivers/staging/lustre/lustre/llite/dir.c
+++ b/drivers/staging/lustre/lustre/llite/dir.c
@@ -239,12 +239,6 @@ static int ll_dir_filler(void *_hash, struct page *page0)
return rc;
 }
 
-static void ll_check_page(struct inode *dir, struct page *page)
-{
-   /* XXX: check page format later */
-   SetPageChecked(page);
-}
-
 void ll_release_page(struct page *page, int remove)
 {
kunmap(page);
@@ -432,7 +426,8 @@ struct page *ll_get_dir_page(struct inode *dir, __u64 hash,
goto fail;
}
if (!PageChecked(page))
-   ll_check_page(dir, page);
+   /* XXX: check page format later */
+   SetPageChecked(page);
if (PageError(page)) {
CERROR("page error: "DFID" at %llu: rc %d\n",
   PFID(ll_inode2fid(dir)), hash, -5);
-- 
2.1.0

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH 1/3] Staging: lustre: module: Replace function calls

[Simmons, James A.]

>From: devel [mailto:driverdev-devel-boun...@linuxdriverproject.org] On Behalf 
>Of Shivani Bhardwaj
>Sent: Friday, November 06, 2015 12:18 PM
>To: gre...@linuxfoundation.org
>Cc: oleg.dro...@intel.com; de...@driverdev.osuosl.org; 
>andreas.dil...@intel.com; linux-kernel@vger.kernel.org
>Subject: [PATCH 1/3] Staging: lustre: module: Replace function calls
>
>Replace the calls of function cfs_trace_free_string_buffer() with
>kfree() as the former function is not required.
>
>Signed-off-by: Shivani Bhardwaj 
>---
> drivers/staging/lustre/lustre/libcfs/module.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)

Acked-by: James Simmons 

diff --git a/drivers/staging/lustre/lustre/libcfs/module.c 
b/drivers/staging/lustre/lustre/libcfs/module.c
index 50e8fd2..516a9e7 100644
--- a/drivers/staging/lustre/lustre/libcfs/module.c
+++ b/drivers/staging/lustre/lustre/libcfs/module.c
@@ -392,7 +392,7 @@ static int __proc_dobitmasks(void *data, int write,
} else {
rc = cfs_trace_copyin_string(tmpstr, tmpstrlen, buffer, nob);
if (rc < 0) {
-   cfs_trace_free_string_buffer(tmpstr, tmpstrlen);
+   kfree(tmpstr);
return rc;
}
 
@@ -402,7 +402,7 @@ static int __proc_dobitmasks(void *data, int write,
*mask |= D_EMERG;
}
 
-   cfs_trace_free_string_buffer(tmpstr, tmpstrlen);
+   kfree(tmpstr);
return rc;
 }
 
-- 
2.1.0

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH 2/3] Staging: lustre: tracefile: Remove wrapper function

[Simmons, James A.]

>-Original Message-
>From: devel [mailto:driverdev-devel-boun...@linuxdriverproject.org] On Behalf 
>Of Shivani Bhardwaj
>Sent: Friday, November 06, 2015 12:19 PM
>To: gre...@linuxfoundation.org
>Cc: oleg.dro...@intel.com; de...@driverdev.osuosl.org; 
>andreas.dil...@intel.com; linux-kernel@vger.kernel.org
>Subject: [PATCH 2/3] Staging: lustre: tracefile: Remove wrapper function
>
>Remove the function cfs_trace_free_string_buffer() as it can be replaced
>with the standard function kfree().
>
>Signed-off-by: Shivani Bhardwaj 
>---
> drivers/staging/lustre/lustre/libcfs/tracefile.c | 9 ++---
> 1 file changed, 2 insertions(+), 7 deletions(-)

Acked-by: James Simmons 

diff --git a/drivers/staging/lustre/lustre/libcfs/tracefile.c 
b/drivers/staging/lustre/lustre/libcfs/tracefile.c
index d55dda8..211047f 100644
--- a/drivers/staging/lustre/lustre/libcfs/tracefile.c
+++ b/drivers/staging/lustre/lustre/libcfs/tracefile.c
@@ -817,11 +817,6 @@ int cfs_trace_allocate_string_buffer(char **str, int nob)
return 0;
 }
 
-void cfs_trace_free_string_buffer(char *str, int nob)
-{
-   kfree(str);
-}
-
 int cfs_trace_dump_debug_buffer_usrstr(void __user *usr_str, int usr_str_nob)
 {
char *str;
@@ -842,7 +837,7 @@ int cfs_trace_dump_debug_buffer_usrstr(void __user 
*usr_str, int usr_str_nob)
}
rc = cfs_tracefile_dump_all_pages(str);
 out:
-   cfs_trace_free_string_buffer(str, usr_str_nob + 1);
+   kfree(str);
return rc;
 }
 
@@ -898,7 +893,7 @@ int cfs_trace_daemon_command_usrstr(void __user *usr_str, 
int usr_str_nob)
if (rc == 0)
rc = cfs_trace_daemon_command(str);
 
-   cfs_trace_free_string_buffer(str, usr_str_nob + 1);
+   kfree(str);
return rc;
 }
 
-- 
2.1.0

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH 1/3] Staging: lustre: ldlm_pool: Remove unneeded wrapper function

[Simmons, James A.]

>-Original Message-
>From: devel [mailto:driverdev-devel-boun...@linuxdriverproject.org] On Behalf 
>Of Shivani Bhardwaj
>Sent: Friday, November 06, 2015 12:43 PM
>To: gre...@linuxfoundation.org
>Cc: oleg.dro...@intel.com; de...@driverdev.osuosl.org; 
>andreas.dil...@intel.com; linux-kernel@vger.kernel.org
>Subject: [PATCH 1/3] Staging: lustre: ldlm_pool: Remove unneeded wrapper 
>function
>
>Remove the function ldlm_pl2ns() and replace its calls with the function
>it wrapped.
>
>Signed-off-by: Shivani Bhardwaj 
>---
> drivers/staging/lustre/lustre/ldlm/ldlm_pool.c | 25 +
> 1 file changed, 13 insertions(+), 12 deletions(-)

Acked-by: James Simmons 

diff --git a/drivers/staging/lustre/lustre/ldlm/ldlm_pool.c 
b/drivers/staging/lustre/lustre/ldlm/ldlm_pool.c
index 1a4eef6..2beb36b 100644
--- a/drivers/staging/lustre/lustre/ldlm/ldlm_pool.c
+++ b/drivers/staging/lustre/lustre/ldlm/ldlm_pool.c
@@ -176,11 +176,6 @@ enum {
LDLM_POOL_LAST_STAT
 };
 
-static inline struct ldlm_namespace *ldlm_pl2ns(struct ldlm_pool *pl)
-{
-   return container_of(pl, struct ldlm_namespace, ns_pool);
-}
-
 /**
  * Calculates suggested grant_step in % of available locks for passed
  * \a period. This is later used in grant_plan calculations.
@@ -254,7 +249,8 @@ static void ldlm_pool_recalc_stats(struct ldlm_pool *pl)
 }
 
 /**
- * Sets SLV and Limit from ldlm_pl2ns(pl)->ns_obd tp passed \a pl.
+ * Sets SLV and Limit from container_of(pl, struct ldlm_namespace,
+ * ns_pool)->ns_obd tp passed \a pl.
  */
 static void ldlm_cli_pool_pop_slv(struct ldlm_pool *pl)
 {
@@ -264,7 +260,8 @@ static void ldlm_cli_pool_pop_slv(struct ldlm_pool *pl)
 * Get new SLV and Limit from obd which is updated with coming
 * RPCs.
 */
-   obd = ldlm_pl2ns(pl)->ns_obd;
+   obd = container_of(pl, struct ldlm_namespace,
+  ns_pool)->ns_obd;
LASSERT(obd != NULL);
read_lock(>obd_pool_lock);
pl->pl_server_lock_volume = obd->obd_pool_slv;
@@ -304,7 +301,8 @@ static int ldlm_cli_pool_recalc(struct ldlm_pool *pl)
/*
 * Do not cancel locks in case lru resize is disabled for this ns.
 */
-   if (!ns_connect_lru_resize(ldlm_pl2ns(pl))) {
+   if (!ns_connect_lru_resize(container_of(pl, struct ldlm_namespace,
+   ns_pool))) {
ret = 0;
goto out;
}
@@ -315,7 +313,8 @@ static int ldlm_cli_pool_recalc(struct ldlm_pool *pl)
 * It may be called when SLV has changed much, this is why we do not
 * take into account pl->pl_recalc_time here.
 */
-   ret = ldlm_cancel_lru(ldlm_pl2ns(pl), 0, LCF_ASYNC, LDLM_CANCEL_LRUR);
+   ret = ldlm_cancel_lru(container_of(pl, struct ldlm_namespace, ns_pool),
+ 0, LCF_ASYNC, LDLM_CANCEL_LRUR);
 
 out:
spin_lock(>pl_lock);
@@ -341,7 +340,7 @@ static int ldlm_cli_pool_shrink(struct ldlm_pool *pl,
struct ldlm_namespace *ns;
int unused;
 
-   ns = ldlm_pl2ns(pl);
+   ns = container_of(pl, struct ldlm_namespace, ns_pool);
 
/*
 * Do not cancel locks in case lru resize is disabled for this ns.
@@ -558,7 +557,8 @@ static struct kobj_type ldlm_pl_ktype = {
 
 static int ldlm_pool_sysfs_init(struct ldlm_pool *pl)
 {
-   struct ldlm_namespace *ns = ldlm_pl2ns(pl);
+   struct ldlm_namespace *ns = container_of(pl, struct ldlm_namespace,
+ns_pool);
int err;
 
init_completion(>pl_kobj_unregister);
@@ -570,7 +570,8 @@ static int ldlm_pool_sysfs_init(struct ldlm_pool *pl)
 
 static int ldlm_pool_debugfs_init(struct ldlm_pool *pl)
 {
-   struct ldlm_namespace *ns = ldlm_pl2ns(pl);
+   struct ldlm_namespace *ns = container_of(pl, struct ldlm_namespace,
+ns_pool);
struct dentry *debugfs_ns_parent;
struct lprocfs_vars pool_vars[2];
char *var_name = NULL;
-- 
2.1.0

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH 2/3] Staging: lustre: ldlm_pool: Drop wrapper function

[Simmons, James A.]

>-Original Message-
>From: devel [mailto:driverdev-devel-boun...@linuxdriverproject.org] On Behalf 
>Of Shivani Bhardwaj
>Sent: Friday, November 06, 2015 12:44 PM
>To: gre...@linuxfoundation.org
>Cc: oleg.dro...@intel.com; de...@driverdev.osuosl.org; 
>andreas.dil...@intel.com; linux-kernel@vger.kernel.org
>Subject: [PATCH 2/3] Staging: lustre: ldlm_pool: Drop wrapper function
>
>Remove the function ldlm_pool_get_limit() and replace its calls with the
>function it wrapped.
>
>Signed-off-by: Shivani Bhardwaj 
>---
> drivers/staging/lustre/lustre/ldlm/ldlm_pool.c | 10 +-
> 1 file changed, 1 insertion(+), 9 deletions(-)

Acked-by: James Simmons 

diff --git a/drivers/staging/lustre/lustre/ldlm/ldlm_pool.c 
b/drivers/staging/lustre/lustre/ldlm/ldlm_pool.c
index 2beb36b..20cf389 100644
--- a/drivers/staging/lustre/lustre/ldlm/ldlm_pool.c
+++ b/drivers/staging/lustre/lustre/ldlm/ldlm_pool.c
@@ -208,14 +208,6 @@ static inline int ldlm_pool_t2gsp(unsigned int t)
 }
 
 /**
- * Returns current \a pl limit.
- */
-static __u32 ldlm_pool_get_limit(struct ldlm_pool *pl)
-{
-   return atomic_read(>pl_limit);
-}
-
-/**
  * Sets passed \a limit to \a pl.
  */
 static void ldlm_pool_set_limit(struct ldlm_pool *pl, __u32 limit)
@@ -452,7 +444,7 @@ static int lprocfs_pool_state_seq_show(struct seq_file *m, 
void *unused)
spin_lock(>pl_lock);
slv = pl->pl_server_lock_volume;
clv = pl->pl_client_lock_volume;
-   limit = ldlm_pool_get_limit(pl);
+   limit = atomic_read(>pl_limit);
granted = atomic_read(>pl_granted);
grant_rate = atomic_read(>pl_grant_rate);
cancel_rate = atomic_read(>pl_cancel_rate);
-- 
2.1.0

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH 3/3] Staging: lustre: ldlm_pool: Drop unneeded wrapper function

[Simmons, James A.]

>-Original Message-
>From: devel [mailto:driverdev-devel-boun...@linuxdriverproject.org] On Behalf 
>Of Shivani Bhardwaj
>Sent: Friday, November 06, 2015 12:45 PM
>To: gre...@linuxfoundation.org
>Cc: oleg.dro...@intel.com; de...@driverdev.osuosl.org; 
>andreas.dil...@intel.com; linux-kernel@vger.kernel.org
>Subject: [PATCH 3/3] Staging: lustre: ldlm_pool: Drop unneeded wrapper function
>
>Remove the function ldlm_pool_set_limit() and replace its calls with the
>function it wrapped.
>
>Signed-off-by: Shivani Bhardwaj 
>---
> drivers/staging/lustre/lustre/ldlm/ldlm_pool.c | 12 ++--
 1 file changed, 2 insertions(+), 10 deletions(-)
>

Acked-by: James Simmons 

diff --git a/drivers/staging/lustre/lustre/ldlm/ldlm_pool.c 
b/drivers/staging/lustre/lustre/ldlm/ldlm_pool.c
index 20cf389..e59b286 100644
--- a/drivers/staging/lustre/lustre/ldlm/ldlm_pool.c
+++ b/drivers/staging/lustre/lustre/ldlm/ldlm_pool.c
@@ -208,14 +208,6 @@ static inline int ldlm_pool_t2gsp(unsigned int t)
 }
 
 /**
- * Sets passed \a limit to \a pl.
- */
-static void ldlm_pool_set_limit(struct ldlm_pool *pl, __u32 limit)
-{
-   atomic_set(>pl_limit, limit);
-}
-
-/**
  * Recalculates next stats on passed \a pl.
  *
  * \pre ->pl_lock is locked.
@@ -257,7 +249,7 @@ static void ldlm_cli_pool_pop_slv(struct ldlm_pool *pl)
LASSERT(obd != NULL);
read_lock(>obd_pool_lock);
pl->pl_server_lock_volume = obd->obd_pool_slv;
-   ldlm_pool_set_limit(pl, obd->obd_pool_limit);
+   atomic_set(>pl_limit, obd->obd_pool_limit);
read_unlock(>obd_pool_lock);
 }
 
@@ -678,7 +670,7 @@ int ldlm_pool_init(struct ldlm_pool *pl, struct 
ldlm_namespace *ns,
snprintf(pl->pl_name, sizeof(pl->pl_name), "ldlm-pool-%s-%d",
 ldlm_ns_name(ns), idx);
 
-   ldlm_pool_set_limit(pl, 1);
+   atomic_set(>pl_limit, 1);
pl->pl_server_lock_volume = 0;
pl->pl_ops = _cli_pool_ops;
pl->pl_recalc_period = LDLM_POOL_CLI_DEF_RECALC_PERIOD;
-- 
2.1.0

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH 3/3] Staging: lustre: ldlm_pool: Drop unneeded wrapper function

[Simmons, James A.]

>-Original Message-
>From: devel [mailto:driverdev-devel-boun...@linuxdriverproject.org] On Behalf 
>Of Shivani Bhardwaj
>Sent: Friday, November 06, 2015 12:45 PM
>To: gre...@linuxfoundation.org
>Cc: oleg.dro...@intel.com; de...@driverdev.osuosl.org; 
>andreas.dil...@intel.com; linux-kernel@vger.kernel.org
>Subject: [PATCH 3/3] Staging: lustre: ldlm_pool: Drop unneeded wrapper function
>
>Remove the function ldlm_pool_set_limit() and replace its calls with the
>function it wrapped.
>
>Signed-off-by: Shivani Bhardwaj 
>---
> drivers/staging/lustre/lustre/ldlm/ldlm_pool.c | 12 ++--
 1 file changed, 2 insertions(+), 10 deletions(-)
>

Acked-by: James Simmons 

diff --git a/drivers/staging/lustre/lustre/ldlm/ldlm_pool.c 
b/drivers/staging/lustre/lustre/ldlm/ldlm_pool.c
index 20cf389..e59b286 100644
--- a/drivers/staging/lustre/lustre/ldlm/ldlm_pool.c
+++ b/drivers/staging/lustre/lustre/ldlm/ldlm_pool.c
@@ -208,14 +208,6 @@ static inline int ldlm_pool_t2gsp(unsigned int t)
 }
 
 /**
- * Sets passed \a limit to \a pl.
- */
-static void ldlm_pool_set_limit(struct ldlm_pool *pl, __u32 limit)
-{
-   atomic_set(>pl_limit, limit);
-}
-
-/**
  * Recalculates next stats on passed \a pl.
  *
  * \pre ->pl_lock is locked.
@@ -257,7 +249,7 @@ static void ldlm_cli_pool_pop_slv(struct ldlm_pool *pl)
LASSERT(obd != NULL);
read_lock(>obd_pool_lock);
pl->pl_server_lock_volume = obd->obd_pool_slv;
-   ldlm_pool_set_limit(pl, obd->obd_pool_limit);
+   atomic_set(>pl_limit, obd->obd_pool_limit);
read_unlock(>obd_pool_lock);
 }
 
@@ -678,7 +670,7 @@ int ldlm_pool_init(struct ldlm_pool *pl, struct 
ldlm_namespace *ns,
snprintf(pl->pl_name, sizeof(pl->pl_name), "ldlm-pool-%s-%d",
 ldlm_ns_name(ns), idx);
 
-   ldlm_pool_set_limit(pl, 1);
+   atomic_set(>pl_limit, 1);
pl->pl_server_lock_volume = 0;
pl->pl_ops = _cli_pool_ops;
pl->pl_recalc_period = LDLM_POOL_CLI_DEF_RECALC_PERIOD;
-- 
2.1.0

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH 1/3] Staging: lustre: ldlm_pool: Remove unneeded wrapper function

[Simmons, James A.]

>-Original Message-
>From: devel [mailto:driverdev-devel-boun...@linuxdriverproject.org] On Behalf 
>Of Shivani Bhardwaj
>Sent: Friday, November 06, 2015 12:43 PM
>To: gre...@linuxfoundation.org
>Cc: oleg.dro...@intel.com; de...@driverdev.osuosl.org; 
>andreas.dil...@intel.com; linux-kernel@vger.kernel.org
>Subject: [PATCH 1/3] Staging: lustre: ldlm_pool: Remove unneeded wrapper 
>function
>
>Remove the function ldlm_pl2ns() and replace its calls with the function
>it wrapped.
>
>Signed-off-by: Shivani Bhardwaj 
>---
> drivers/staging/lustre/lustre/ldlm/ldlm_pool.c | 25 +
> 1 file changed, 13 insertions(+), 12 deletions(-)

Acked-by: James Simmons 

diff --git a/drivers/staging/lustre/lustre/ldlm/ldlm_pool.c 
b/drivers/staging/lustre/lustre/ldlm/ldlm_pool.c
index 1a4eef6..2beb36b 100644
--- a/drivers/staging/lustre/lustre/ldlm/ldlm_pool.c
+++ b/drivers/staging/lustre/lustre/ldlm/ldlm_pool.c
@@ -176,11 +176,6 @@ enum {
LDLM_POOL_LAST_STAT
 };
 
-static inline struct ldlm_namespace *ldlm_pl2ns(struct ldlm_pool *pl)
-{
-   return container_of(pl, struct ldlm_namespace, ns_pool);
-}
-
 /**
  * Calculates suggested grant_step in % of available locks for passed
  * \a period. This is later used in grant_plan calculations.
@@ -254,7 +249,8 @@ static void ldlm_pool_recalc_stats(struct ldlm_pool *pl)
 }
 
 /**
- * Sets SLV and Limit from ldlm_pl2ns(pl)->ns_obd tp passed \a pl.
+ * Sets SLV and Limit from container_of(pl, struct ldlm_namespace,
+ * ns_pool)->ns_obd tp passed \a pl.
  */
 static void ldlm_cli_pool_pop_slv(struct ldlm_pool *pl)
 {
@@ -264,7 +260,8 @@ static void ldlm_cli_pool_pop_slv(struct ldlm_pool *pl)
 * Get new SLV and Limit from obd which is updated with coming
 * RPCs.
 */
-   obd = ldlm_pl2ns(pl)->ns_obd;
+   obd = container_of(pl, struct ldlm_namespace,
+  ns_pool)->ns_obd;
LASSERT(obd != NULL);
read_lock(>obd_pool_lock);
pl->pl_server_lock_volume = obd->obd_pool_slv;
@@ -304,7 +301,8 @@ static int ldlm_cli_pool_recalc(struct ldlm_pool *pl)
/*
 * Do not cancel locks in case lru resize is disabled for this ns.
 */
-   if (!ns_connect_lru_resize(ldlm_pl2ns(pl))) {
+   if (!ns_connect_lru_resize(container_of(pl, struct ldlm_namespace,
+   ns_pool))) {
ret = 0;
goto out;
}
@@ -315,7 +313,8 @@ static int ldlm_cli_pool_recalc(struct ldlm_pool *pl)
 * It may be called when SLV has changed much, this is why we do not
 * take into account pl->pl_recalc_time here.
 */
-   ret = ldlm_cancel_lru(ldlm_pl2ns(pl), 0, LCF_ASYNC, LDLM_CANCEL_LRUR);
+   ret = ldlm_cancel_lru(container_of(pl, struct ldlm_namespace, ns_pool),
+ 0, LCF_ASYNC, LDLM_CANCEL_LRUR);
 
 out:
spin_lock(>pl_lock);
@@ -341,7 +340,7 @@ static int ldlm_cli_pool_shrink(struct ldlm_pool *pl,
struct ldlm_namespace *ns;
int unused;
 
-   ns = ldlm_pl2ns(pl);
+   ns = container_of(pl, struct ldlm_namespace, ns_pool);
 
/*
 * Do not cancel locks in case lru resize is disabled for this ns.
@@ -558,7 +557,8 @@ static struct kobj_type ldlm_pl_ktype = {
 
 static int ldlm_pool_sysfs_init(struct ldlm_pool *pl)
 {
-   struct ldlm_namespace *ns = ldlm_pl2ns(pl);
+   struct ldlm_namespace *ns = container_of(pl, struct ldlm_namespace,
+ns_pool);
int err;
 
init_completion(>pl_kobj_unregister);
@@ -570,7 +570,8 @@ static int ldlm_pool_sysfs_init(struct ldlm_pool *pl)
 
 static int ldlm_pool_debugfs_init(struct ldlm_pool *pl)
 {
-   struct ldlm_namespace *ns = ldlm_pl2ns(pl);
+   struct ldlm_namespace *ns = container_of(pl, struct ldlm_namespace,
+ns_pool);
struct dentry *debugfs_ns_parent;
struct lprocfs_vars pool_vars[2];
char *var_name = NULL;
-- 
2.1.0

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH 2/3] Staging: lustre: ldlm_pool: Drop wrapper function

[Simmons, James A.]

>-Original Message-
>From: devel [mailto:driverdev-devel-boun...@linuxdriverproject.org] On Behalf 
>Of Shivani Bhardwaj
>Sent: Friday, November 06, 2015 12:44 PM
>To: gre...@linuxfoundation.org
>Cc: oleg.dro...@intel.com; de...@driverdev.osuosl.org; 
>andreas.dil...@intel.com; linux-kernel@vger.kernel.org
>Subject: [PATCH 2/3] Staging: lustre: ldlm_pool: Drop wrapper function
>
>Remove the function ldlm_pool_get_limit() and replace its calls with the
>function it wrapped.
>
>Signed-off-by: Shivani Bhardwaj 
>---
> drivers/staging/lustre/lustre/ldlm/ldlm_pool.c | 10 +-
> 1 file changed, 1 insertion(+), 9 deletions(-)

Acked-by: James Simmons 

diff --git a/drivers/staging/lustre/lustre/ldlm/ldlm_pool.c 
b/drivers/staging/lustre/lustre/ldlm/ldlm_pool.c
index 2beb36b..20cf389 100644
--- a/drivers/staging/lustre/lustre/ldlm/ldlm_pool.c
+++ b/drivers/staging/lustre/lustre/ldlm/ldlm_pool.c
@@ -208,14 +208,6 @@ static inline int ldlm_pool_t2gsp(unsigned int t)
 }
 
 /**
- * Returns current \a pl limit.
- */
-static __u32 ldlm_pool_get_limit(struct ldlm_pool *pl)
-{
-   return atomic_read(>pl_limit);
-}
-
-/**
  * Sets passed \a limit to \a pl.
  */
 static void ldlm_pool_set_limit(struct ldlm_pool *pl, __u32 limit)
@@ -452,7 +444,7 @@ static int lprocfs_pool_state_seq_show(struct seq_file *m, 
void *unused)
spin_lock(>pl_lock);
slv = pl->pl_server_lock_volume;
clv = pl->pl_client_lock_volume;
-   limit = ldlm_pool_get_limit(pl);
+   limit = atomic_read(>pl_limit);
granted = atomic_read(>pl_granted);
grant_rate = atomic_read(>pl_grant_rate);
cancel_rate = atomic_read(>pl_cancel_rate);
-- 
2.1.0

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH 2/3] Staging: lustre: tracefile: Remove wrapper function

[Simmons, James A.]

>-Original Message-
>From: devel [mailto:driverdev-devel-boun...@linuxdriverproject.org] On Behalf 
>Of Shivani Bhardwaj
>Sent: Friday, November 06, 2015 12:19 PM
>To: gre...@linuxfoundation.org
>Cc: oleg.dro...@intel.com; de...@driverdev.osuosl.org; 
>andreas.dil...@intel.com; linux-kernel@vger.kernel.org
>Subject: [PATCH 2/3] Staging: lustre: tracefile: Remove wrapper function
>
>Remove the function cfs_trace_free_string_buffer() as it can be replaced
>with the standard function kfree().
>
>Signed-off-by: Shivani Bhardwaj 
>---
> drivers/staging/lustre/lustre/libcfs/tracefile.c | 9 ++---
> 1 file changed, 2 insertions(+), 7 deletions(-)

Acked-by: James Simmons 

diff --git a/drivers/staging/lustre/lustre/libcfs/tracefile.c 
b/drivers/staging/lustre/lustre/libcfs/tracefile.c
index d55dda8..211047f 100644
--- a/drivers/staging/lustre/lustre/libcfs/tracefile.c
+++ b/drivers/staging/lustre/lustre/libcfs/tracefile.c
@@ -817,11 +817,6 @@ int cfs_trace_allocate_string_buffer(char **str, int nob)
return 0;
 }
 
-void cfs_trace_free_string_buffer(char *str, int nob)
-{
-   kfree(str);
-}
-
 int cfs_trace_dump_debug_buffer_usrstr(void __user *usr_str, int usr_str_nob)
 {
char *str;
@@ -842,7 +837,7 @@ int cfs_trace_dump_debug_buffer_usrstr(void __user 
*usr_str, int usr_str_nob)
}
rc = cfs_tracefile_dump_all_pages(str);
 out:
-   cfs_trace_free_string_buffer(str, usr_str_nob + 1);
+   kfree(str);
return rc;
 }
 
@@ -898,7 +893,7 @@ int cfs_trace_daemon_command_usrstr(void __user *usr_str, 
int usr_str_nob)
if (rc == 0)
rc = cfs_trace_daemon_command(str);
 
-   cfs_trace_free_string_buffer(str, usr_str_nob + 1);
+   kfree(str);
return rc;
 }
 
-- 
2.1.0

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH 1/3] Staging: lustre: module: Replace function calls

[Simmons, James A.]

>From: devel [mailto:driverdev-devel-boun...@linuxdriverproject.org] On Behalf 
>Of Shivani Bhardwaj
>Sent: Friday, November 06, 2015 12:18 PM
>To: gre...@linuxfoundation.org
>Cc: oleg.dro...@intel.com; de...@driverdev.osuosl.org; 
>andreas.dil...@intel.com; linux-kernel@vger.kernel.org
>Subject: [PATCH 1/3] Staging: lustre: module: Replace function calls
>
>Replace the calls of function cfs_trace_free_string_buffer() with
>kfree() as the former function is not required.
>
>Signed-off-by: Shivani Bhardwaj 
>---
> drivers/staging/lustre/lustre/libcfs/module.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)

Acked-by: James Simmons 

diff --git a/drivers/staging/lustre/lustre/libcfs/module.c 
b/drivers/staging/lustre/lustre/libcfs/module.c
index 50e8fd2..516a9e7 100644
--- a/drivers/staging/lustre/lustre/libcfs/module.c
+++ b/drivers/staging/lustre/lustre/libcfs/module.c
@@ -392,7 +392,7 @@ static int __proc_dobitmasks(void *data, int write,
} else {
rc = cfs_trace_copyin_string(tmpstr, tmpstrlen, buffer, nob);
if (rc < 0) {
-   cfs_trace_free_string_buffer(tmpstr, tmpstrlen);
+   kfree(tmpstr);
return rc;
}
 
@@ -402,7 +402,7 @@ static int __proc_dobitmasks(void *data, int write,
*mask |= D_EMERG;
}
 
-   cfs_trace_free_string_buffer(tmpstr, tmpstrlen);
+   kfree(tmpstr);
return rc;
 }
 
-- 
2.1.0

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH] Staging: lustre: dir: Remove wrapper function

[Simmons, James A.]

>From: devel [mailto:driverdev-devel-boun...@linuxdriverproject.org] On Behalf 
>Of Shivani Bhardwaj
>Sent: Friday, November 06, 2015 11:55 AM
>To: gre...@linuxfoundation.org
>Cc: oleg.dro...@intel.com; de...@driverdev.osuosl.org; 
>andreas.dil...@intel.com; linux-kernel@vger.kernel.org; 
>lustre-de...@lists.lustre.org
>Subject: [PATCH] Staging: lustre: dir: Remove wrapper function
>
>Remove the function ll_check_page() and replace all its calls with the
>function it wrapped.
>
>Signed-off-by: Shivani Bhardwaj 
>---
> drivers/staging/lustre/lustre/llite/dir.c | 9 ++---
> 1 file changed, 2 insertions(+), 7 deletions(-)

Acked-by: James Simmons 

diff --git a/drivers/staging/lustre/lustre/llite/dir.c 
b/drivers/staging/lustre/lustre/llite/dir.c
index 5c9502b..842e6d6 100644
--- a/drivers/staging/lustre/lustre/llite/dir.c
+++ b/drivers/staging/lustre/lustre/llite/dir.c
@@ -239,12 +239,6 @@ static int ll_dir_filler(void *_hash, struct page *page0)
return rc;
 }
 
-static void ll_check_page(struct inode *dir, struct page *page)
-{
-   /* XXX: check page format later */
-   SetPageChecked(page);
-}
-
 void ll_release_page(struct page *page, int remove)
 {
kunmap(page);
@@ -432,7 +426,8 @@ struct page *ll_get_dir_page(struct inode *dir, __u64 hash,
goto fail;
}
if (!PageChecked(page))
-   ll_check_page(dir, page);
+   /* XXX: check page format later */
+   SetPageChecked(page);
if (PageError(page)) {
CERROR("page error: "DFID" at %llu: rc %d\n",
   PFID(ll_inode2fid(dir)), hash, -5);
-- 
2.1.0

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [lustre-devel] [PATCH] staging: lustre: lnet: klnds: socklnd: Move extern declarations to header

[Simmons, James A.]

>From: lustre-devel [mailto:lustre-devel-boun...@lists.lustre.org] On Behalf Of 
>Amitoj Kaur Chawla
>Sent: Friday, November 06, 2015 9:57 AM
>To: oleg.dro...@intel.com; andreas.dil...@intel.com; 
>gre...@linuxfoundation.org; lustre-de...@lists.lustre.org; 
>de...@driverdev.osuosl.org; linux-kernel@vger.kernel.org
>Subject: [lustre-devel] [PATCH] staging: lustre: lnet: klnds: socklnd: Move 
>extern declarations to header
>
>This patch moves extern declarations in socklnd_lib.c to the respective 
>header file, 'socklnd.h'.
>
>This patch also removes extern keyword from function declarations
>since functions have the extern specifier by default.
>
>Signed-off-by: Amitoj Kaur Chawla 
>---
> drivers/staging/lustre/lnet/klnds/socklnd/socklnd.h | 3 +++
> drivers/staging/lustre/lnet/klnds/socklnd/socklnd_lib.c | 2 --
> 2 files changed, 3 insertions(+), 2 deletions(-)

Acked-by: James Simmons 

diff --git a/drivers/staging/lustre/lnet/klnds/socklnd/socklnd.h 
b/drivers/staging/lustre/lnet/klnds/socklnd/socklnd.h
index b349847..f4fa725 100644
--- a/drivers/staging/lustre/lnet/klnds/socklnd/socklnd.h
+++ b/drivers/staging/lustre/lnet/klnds/socklnd/socklnd.h
@@ -679,6 +679,9 @@ int ksocknal_lib_recv_kiov(ksock_conn_t *conn);
 int ksocknal_lib_get_conn_tunables(ksock_conn_t *conn, int *txmem,
   int *rxmem, int *nagle);
 
+void ksocknal_read_callback(ksock_conn_t *conn);
+void ksocknal_write_callback(ksock_conn_t *conn);
+
 int ksocknal_tunables_init(void);
 
 void ksocknal_lib_csum_tx(ksock_tx_t *tx);
diff --git a/drivers/staging/lustre/lnet/klnds/socklnd/socklnd_lib.c 
b/drivers/staging/lustre/lnet/klnds/socklnd/socklnd_lib.c
index 679785b..04a4653 100644
--- a/drivers/staging/lustre/lnet/klnds/socklnd/socklnd_lib.c
+++ b/drivers/staging/lustre/lnet/klnds/socklnd/socklnd_lib.c
@@ -580,8 +580,6 @@ ksocknal_lib_push_conn(ksock_conn_t *conn)
ksocknal_connsock_decref(conn);
 }
 
-extern void ksocknal_read_callback(ksock_conn_t *conn);
-extern void ksocknal_write_callback(ksock_conn_t *conn);
 /*
  * socket call back in Linux
  */
-- 
1.9.1

___
lustre-devel mailing list
lustre-de...@lists.lustre.org
http://lists.lustre.org/listinfo.cgi/lustre-devel-lustre.org

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [lustre-devel] [PATCH 07/10] staging: lustre: Handle nodemask on UMP machines

[Simmons, James A.]

>All warnings (new ones prefixed by >>):
>
>   In file included from include/linux/bitops.h:36:0,
>from 
> drivers/staging/lustre/lustre/libcfs/../../include/linux/libcfs/linux/libcfs.h:44,
>from 
> drivers/staging/lustre/lustre/libcfs/../../include/linux/libcfs/libcfs.h:40,
>from drivers/staging/lustre/lustre/libcfs/libcfs_cpu.c:38:
>   drivers/staging/lustre/lustre/libcfs/libcfs_cpu.c: In function 
> 'cfs_cpt_table_alloc':
>>> arch/m68k/include/asm/bitops.h:64:5: warning: passing argument 2 of 
>>> 'bset_mem_set_bit' from incompatible pointer type
>bset_mem_set_bit(nr, vaddr) : \
>^
>>> drivers/staging/lustre/lustre/libcfs/libcfs_cpu.c:61:3: note: in expansion 
>>> of macro 'set_bit'
>  set_bit(0, >ctb_nodemask);

Yep and additional patch exist to fix this. Should I just push the fix for this 
or drop this patch and create
a new patch that is combo of both fixes.

BTW Greg this new batch of patches are order independent. Sorry for not 
pointing that out. The rest of the
patch appear to be okay.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [lustre-devel] [PATCH v2 1/7] staging: lustre: remove white space in libcfs_hash.h

[Simmons, James A.]

>On Tue, Nov 03, 2015 at 07:46:07PM -0800, Greg Kroah-Hartman wrote:
>> On Mon, Nov 02, 2015 at 12:22:07PM -0500, James Simmons wrote:
>> > Cleanup all the unneeded white space in libcfs_hash.h.
>> > 
>> > Signed-off-by: James Simmons 
>> > ---
>> >  .../lustre/include/linux/libcfs/libcfs_hash.h  |  135 
>> > ++--
>> >  1 files changed, 70 insertions(+), 65 deletions(-)
>> 
>> Doesn't apply, did I already queue up this series?
>
>No. This did not apply because of:
>c7fdf4a3959f ("staging: lustre: fix remaining checkpatch issues for 
>libcfs_hash.h")

Surprise this was merged which is why I did a second series for this.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH v2 1/7] staging: lustre: remove white space in libcfs_hash.h

[Simmons, James A.]

>On Mon, Nov 02, 2015 at 12:22:07PM -0500, James Simmons wrote:
>> Cleanup all the unneeded white space in libcfs_hash.h.
>> 
>> Signed-off-by: James Simmons 
>> ---
>>  .../lustre/include/linux/libcfs/libcfs_hash.h  |  135 
>> ++--
>>  1 files changed, 70 insertions(+), 65 deletions(-)
>
>Doesn't apply, did I already queue up this series?

I did a second version of those patches. In one batch you will notice [PATCH 
v2].
The reason I did a second batch was to break up the "fix checkpatch issues" 
patch
in the first series. I was trying to behave :-)
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [lustre-devel] [PATCH v2 1/7] staging: lustre: remove white space in libcfs_hash.h

[Simmons, James A.]

>On Tue, Nov 03, 2015 at 07:46:07PM -0800, Greg Kroah-Hartman wrote:
>> On Mon, Nov 02, 2015 at 12:22:07PM -0500, James Simmons wrote:
>> > Cleanup all the unneeded white space in libcfs_hash.h.
>> > 
>> > Signed-off-by: James Simmons 
>> > ---
>> >  .../lustre/include/linux/libcfs/libcfs_hash.h  |  135 
>> > ++--
>> >  1 files changed, 70 insertions(+), 65 deletions(-)
>> 
>> Doesn't apply, did I already queue up this series?
>
>No. This did not apply because of:
>c7fdf4a3959f ("staging: lustre: fix remaining checkpatch issues for 
>libcfs_hash.h")

Surprise this was merged which is why I did a second series for this.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH v2 1/7] staging: lustre: remove white space in libcfs_hash.h

[Simmons, James A.]

>On Mon, Nov 02, 2015 at 12:22:07PM -0500, James Simmons wrote:
>> Cleanup all the unneeded white space in libcfs_hash.h.
>> 
>> Signed-off-by: James Simmons 
>> ---
>>  .../lustre/include/linux/libcfs/libcfs_hash.h  |  135 
>> ++--
>>  1 files changed, 70 insertions(+), 65 deletions(-)
>
>Doesn't apply, did I already queue up this series?

I did a second version of those patches. In one batch you will notice [PATCH 
v2].
The reason I did a second batch was to break up the "fix checkpatch issues" 
patch
in the first series. I was trying to behave :-)
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH] staging:lustre: Prevent duplicate CT registrations

[Simmons, James A.]

>On Fri, Oct 23, 2015 at 03:59:14PM -0400, James Simmons wrote:
>> diff --git a/drivers/staging/lustre/lustre/lmv/lmv_obd.c 
>> b/drivers/staging/lustre/lustre/lmv/lmv_obd.c
>> index 635a93c..d6d70d8 100644
>> --- a/drivers/staging/lustre/lustre/lmv/lmv_obd.c
>> +++ b/drivers/staging/lustre/lustre/lmv/lmv_obd.c
>> @@ -794,7 +794,9 @@ static void lmv_hsm_req_build(struct lmv_obd *lmv,
>>  static int lmv_hsm_ct_unregister(struct lmv_obd *lmv, unsigned int cmd, int 
>> len,
>>   struct lustre_kernelcomm *lk, void *uarg)
>>  {
>> -int i, rc = 0;
>> +struct kkuc_ct_data *kcd = NULL;
>> +int rc = 0;
>> +__u32 i;
>
>We have been introducing a lot of new __u32 types here and I just
>assumed there was a reason for it but this one is clearly wrong.  The
>new code implies that ->ld_tgt_count can overflow INT_MAX which is not
>true and that this is code shared with userspace which might be true but
>it's not described in the changelog.  Is this a static checker fix?
>Stop using that broken static checker, because the correct type here is
>int.

No this patch is from a real fix. You can read the details at
https://jira.hpdd.intel.com/browse/LU-3882. I bet he did this to avoid
the pickiness of gcc. We have build failure due to gcc stupidity. I could
be wrong which if that is the case the original Author is CC.

>Anyway, stop making gratuitous unrelated changes (like the white space
>changes to local declarations).  I feel like I have held off commenting
>on this for a while and shown great restraint.  :P

This is a direct drop of the original patch. I see lots of corrections to the
code below. Would a new follow on patch to correct these issues be okay
with you. This way we can perverse our bug fix history.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [lustre-devel] [PATCH 1/3] staging: lustre: checkpatch cleanups for nidstring.c

[Simmons, James A.]

>Yeah.  That is often the fastest way to fix all the checkpatch warnings.
>
>Checkpatch warnings are pretty mechanical.  Just send like 100 patches
>at a time until everything is fixed.  Don't overthink.  Say your patch
>breaks the alignment then you have to fix that, but otherwise only fix
>one thing at a time.  Sometimes people will ask you to fix something
>else on the same line, but just say "I didn't introduce that, but yes I
>am planning to fix that in a later patchset since I am following the
>one thing per patch rule."
>
>Don't feel shame about sending many small patches.  We pretty much merge
>everything.

It was the sense of it taking forever with that amount of patches needed with
the one file approach. Looking at the back log of fixes its not as bad as I 
thought
for libcfs/LNet. Once those fixes are merged the style cleanups can happen
pretty quickly.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [lustre-devel] [PATCH 1/3] staging: lustre: checkpatch cleanups for nidstring.c

[Simmons, James A.]

>Yeah.  That is often the fastest way to fix all the checkpatch warnings.
>
>Checkpatch warnings are pretty mechanical.  Just send like 100 patches
>at a time until everything is fixed.  Don't overthink.  Say your patch
>breaks the alignment then you have to fix that, but otherwise only fix
>one thing at a time.  Sometimes people will ask you to fix something
>else on the same line, but just say "I didn't introduce that, but yes I
>am planning to fix that in a later patchset since I am following the
>one thing per patch rule."
>
>Don't feel shame about sending many small patches.  We pretty much merge
>everything.

It was the sense of it taking forever with that amount of patches needed with
the one file approach. Looking at the back log of fixes its not as bad as I 
thought
for libcfs/LNet. Once those fixes are merged the style cleanups can happen
pretty quickly.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH] staging:lustre: Prevent duplicate CT registrations

[Simmons, James A.]

>On Fri, Oct 23, 2015 at 03:59:14PM -0400, James Simmons wrote:
>> diff --git a/drivers/staging/lustre/lustre/lmv/lmv_obd.c 
>> b/drivers/staging/lustre/lustre/lmv/lmv_obd.c
>> index 635a93c..d6d70d8 100644
>> --- a/drivers/staging/lustre/lustre/lmv/lmv_obd.c
>> +++ b/drivers/staging/lustre/lustre/lmv/lmv_obd.c
>> @@ -794,7 +794,9 @@ static void lmv_hsm_req_build(struct lmv_obd *lmv,
>>  static int lmv_hsm_ct_unregister(struct lmv_obd *lmv, unsigned int cmd, int 
>> len,
>>   struct lustre_kernelcomm *lk, void *uarg)
>>  {
>> -int i, rc = 0;
>> +struct kkuc_ct_data *kcd = NULL;
>> +int rc = 0;
>> +__u32 i;
>
>We have been introducing a lot of new __u32 types here and I just
>assumed there was a reason for it but this one is clearly wrong.  The
>new code implies that ->ld_tgt_count can overflow INT_MAX which is not
>true and that this is code shared with userspace which might be true but
>it's not described in the changelog.  Is this a static checker fix?
>Stop using that broken static checker, because the correct type here is
>int.

No this patch is from a real fix. You can read the details at
https://jira.hpdd.intel.com/browse/LU-3882. I bet he did this to avoid
the pickiness of gcc. We have build failure due to gcc stupidity. I could
be wrong which if that is the case the original Author is CC.

>Anyway, stop making gratuitous unrelated changes (like the white space
>changes to local declarations).  I feel like I have held off commenting
>on this for a while and shown great restraint.  :P

This is a direct drop of the original patch. I see lots of corrections to the
code below. Would a new follow on patch to correct these issues be okay
with you. This way we can perverse our bug fix history.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [lustre-devel] [PATCH 1/3] staging: lustre: checkpatch cleanups for nidstring.c

[Simmons, James A.]

>On Thu, Oct 29, 2015 at 07:28:21PM -0400, James Simmons wrote:
>> With nidstring now having the latest fixes we can
>> now clean up all the remaining checkpatch errors
>> for nidstring.c.
>
>Please be specific as to exactly what you changed, and break it up into
>one-patch-per-thing.  And no, "fix all checkpatch errors" is not "one
>thing"

Hmm. This makes me think I might be going about this wrong.  Instead of
doing style changes per file I should be doing one style change per subsystem
instead. Unless you prefer doing these style changes on per file base. Perhaps
for now I should focus on pushing the fixes that have cumulated and once
caught up then finished off the style issues. 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [lustre-devel] [PATCH 1/3] staging: lustre: checkpatch cleanups for nidstring.c

[Simmons, James A.]

>On Thu, Oct 29, 2015 at 07:28:21PM -0400, James Simmons wrote:
>> With nidstring now having the latest fixes we can
>> now clean up all the remaining checkpatch errors
>> for nidstring.c.
>
>Please be specific as to exactly what you changed, and break it up into
>one-patch-per-thing.  And no, "fix all checkpatch errors" is not "one
>thing"

Hmm. This makes me think I might be going about this wrong.  Instead of
doing style changes per file I should be doing one style change per subsystem
instead. Unless you prefer doing these style changes on per file base. Perhaps
for now I should focus on pushing the fixes that have cumulated and once
caught up then finished off the style issues. 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [lustre-devel] [PATCH 09/10] staging: lustre: fix remaining checkpatch issues for libcfs_hash.h

[Simmons, James A.]

>>diff --git a/drivers/staging/lustre/include/linux/libcfs/libcfs_hash.h
>>b/drivers/staging/lustre/include/linux/libcfs/libcfs_hash.h
>>index 5df8ba2..563b2b4 100644
>>--- a/drivers/staging/lustre/include/linux/libcfs/libcfs_hash.h
>>+++ b/drivers/staging/lustre/include/linux/libcfs/libcfs_hash.h
>>@@ -62,7 +62,8 @@
>> /** disable debug */
>> #define CFS_HASH_DEBUG_NONE  0
>> /** record hash depth and output to console when it's too deep,
>>- *  computing overhead is low but consume more memory */
>>+ *  computing overhead is low but consume more memory
>>+ */
>
>Typically, multi-line comments have the leading /* on a separate line
>from the first line of text.  If you are changing all these comments
>you may as well make it consistent with the kernel style.

Fixed for next patch series.


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [lustre-devel] [PATCH 08/10] staging: lustre: remove white space in libcfs_hash.h

[Simmons, James A.]

>>struct cfs_hash_bd {
>>- struct cfs_hash_bucket  *bd_bucket;  /**< address of bucket */
>>- unsigned intbd_offset;  /**< offset in bucket */
>>+ /**< address of bucket */
>>+ struct cfs_hash_bucket  *bd_bucket;
>>+ /**< offset in bucket */
>>+ unsigned int bd_offset;
>> };
>
>The "/**< ... */" marker means "the field to the left", but if you are
>moving these to the line before the field you should just use "/* ... */".

Fixed.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [lustre-devel] [PATCH 08/10] staging: lustre: remove white space in libcfs_hash.h

[Simmons, James A.]

>>struct cfs_hash_bd {
>>- struct cfs_hash_bucket  *bd_bucket;  /**< address of bucket */
>>- unsigned intbd_offset;  /**< offset in bucket */
>>+ /**< address of bucket */
>>+ struct cfs_hash_bucket  *bd_bucket;
>>+ /**< offset in bucket */
>>+ unsigned int bd_offset;
>> };
>
>The "/**< ... */" marker means "the field to the left", but if you are
>moving these to the line before the field you should just use "/* ... */".

Fixed.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [lustre-devel] [PATCH 09/10] staging: lustre: fix remaining checkpatch issues for libcfs_hash.h

[Simmons, James A.]

>>diff --git a/drivers/staging/lustre/include/linux/libcfs/libcfs_hash.h
>>b/drivers/staging/lustre/include/linux/libcfs/libcfs_hash.h
>>index 5df8ba2..563b2b4 100644
>>--- a/drivers/staging/lustre/include/linux/libcfs/libcfs_hash.h
>>+++ b/drivers/staging/lustre/include/linux/libcfs/libcfs_hash.h
>>@@ -62,7 +62,8 @@
>> /** disable debug */
>> #define CFS_HASH_DEBUG_NONE  0
>> /** record hash depth and output to console when it's too deep,
>>- *  computing overhead is low but consume more memory */
>>+ *  computing overhead is low but consume more memory
>>+ */
>
>Typically, multi-line comments have the leading /* on a separate line
>from the first line of text.  If you are changing all these comments
>you may as well make it consistent with the kernel style.

Fixed for next patch series.


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH 2/2] Staging: lustre: lnet: lib-move return of an errno should typically be negative (ie: return -EAGAIN)

[Simmons, James A.]

>Fixed- Return of an errno should typically be negative (ie: return -EAGAIN)
>
>Signed-off-by: Nilesh Kokane 
>---
> drivers/staging/lustre/lnet/lnet/lib-move.c | 22 +++---
> 1 file changed, 11 insertions(+), 11 deletions(-)

Bad idea. I also made this mistake and it broke LNet really badly.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH 2/2] Staging: lustre: lnet: lib-move return of an errno should typically be negative (ie: return -EAGAIN)

[Simmons, James A.]

>Fixed- Return of an errno should typically be negative (ie: return -EAGAIN)
>
>Signed-off-by: Nilesh Kokane 
>---
> drivers/staging/lustre/lnet/lnet/lib-move.c | 22 +++---
> 1 file changed, 11 insertions(+), 11 deletions(-)

Bad idea. I also made this mistake and it broke LNet really badly.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [lustre-devel] [PATCH] staging/lustre: fix block comment formatting

[Simmons, James A.]

>Running checkpatch.pl on lnet/klnds/o2iblnd/o2iblnd.h produces several
>"Block comments use a trailing */ on a separate line" warnings. This patch
>fixes these.

These fixes are just plain ugly. Is this change a requirement?

 .../staging/lustre/lnet/klnds/o2iblnd/o2iblnd.h| 90 ++
 1 file changed, 60 insertions(+), 30 deletions(-)

diff --git a/drivers/staging/lustre/lnet/klnds/o2iblnd/o2iblnd.h 
b/drivers/staging/lustre/lnet/klnds/o2iblnd/o2iblnd.h
index f4b6c33..659d9cc 100644
--- a/drivers/staging/lustre/lnet/klnds/o2iblnd/o2iblnd.h
+++ b/drivers/staging/lustre/lnet/klnds/o2iblnd/o2iblnd.h
@@ -82,9 +82,11 @@ typedef struct {
int  *kib_dev_failover;  /* HCA failover */
unsigned int *kib_service;   /* IB service number */
int  *kib_min_reconnect_interval; /* first failed connection
-  * retry... */
+  * retry...
+  */
int  *kib_max_reconnect_interval; /* ...exponentially increasing
-  * to this */
+  * to this
+  */
int  *kib_cksum; /* checksum kib_msg_t? */
int  *kib_timeout;   /* comms timeout (seconds) */
int  *kib_keepalive; /* keepalive timeout (seconds) */
@@ -92,9 +94,11 @@ typedef struct {
int  *kib_credits;   /* # concurrent sends */
int  *kib_peertxcredits; /* # concurrent sends to 1 peer */
int  *kib_peerrtrcredits;/* # per-peer router buffer
- * credits */
+ * credits
+ */
int  *kib_peercredits_hiw;   /* # when eagerly to return
- * credits */
+ * credits
+ */
int  *kib_peertimeout;   /* seconds to consider peer dead */
char **kib_default_ipif; /* default IPoIB interface */
int  *kib_retry_count;
@@ -103,13 +107,15 @@ typedef struct {
int  *kib_ib_mtu;/* IB MTU */
int  *kib_map_on_demand; /* map-on-demand if RD has more
  * fragments than this value, 0
- * disable map-on-demand */
+ * disable map-on-demand
+ */
int  *kib_fmr_pool_size; /* # FMRs in pool */
int  *kib_fmr_flush_trigger; /* When to trigger FMR flush */
int  *kib_fmr_cache; /* enable FMR pool cache? */
int  *kib_require_priv_port; /* accept only privileged ports */
int  *kib_use_priv_port; /* use privileged port for active
- * connect */
+ * connect
+ */
int  *kib_nscheds;   /* # threads on each CPT */
 } kib_tunables_t;
 
@@ -200,7 +206,8 @@ typedef struct {
intibd_failed_failover; /* # failover failures */
unsigned int   ibd_failover;/* failover in progress */
unsigned int   ibd_can_failover;/* IPoIB interface is a bonding
-* master */
+* master
+*/
struct list_head   ibd_nets;
struct kib_hca_dev *ibd_hdev;
 } kib_dev_t;
@@ -250,7 +257,8 @@ typedef struct kib_poolset {
struct list_head  ps_pool_list;   /* list of pools */
struct list_head  ps_failed_pool_list;/* failed pool list */
unsigned long ps_next_retry;  /* time stamp for retry if
-  * failed to allocate */
+  * failed to allocate
+  */
int   ps_increasing;  /* is allocating new pool */
int   ps_pool_size;   /* new pool size */
int   ps_cpt; /* CPT id */
@@ -258,7 +266,8 @@ typedef struct kib_poolset {
kib_ps_pool_create_t  ps_pool_create; /* create a new pool */
kib_ps_pool_destroy_t ps_pool_destroy;/* destroy a pool */
kib_ps_node_init_tps_node_init;   /* initialize new allocated
- 

RE: [lustre-devel] [PATCH] staging/lustre: fix block comment formatting

[Simmons, James A.]

Running checkpatch.pl on lnet/klnds/o2iblnd/o2iblnd.h produces several
Block comments use a trailing */ on a separate line warnings. This patch
fixes these.

These fixes are just plain ugly. Is this change a requirement?

 .../staging/lustre/lnet/klnds/o2iblnd/o2iblnd.h| 90 ++
 1 file changed, 60 insertions(+), 30 deletions(-)

diff --git a/drivers/staging/lustre/lnet/klnds/o2iblnd/o2iblnd.h 
b/drivers/staging/lustre/lnet/klnds/o2iblnd/o2iblnd.h
index f4b6c33..659d9cc 100644
--- a/drivers/staging/lustre/lnet/klnds/o2iblnd/o2iblnd.h
+++ b/drivers/staging/lustre/lnet/klnds/o2iblnd/o2iblnd.h
@@ -82,9 +82,11 @@ typedef struct {
int  *kib_dev_failover;  /* HCA failover */
unsigned int *kib_service;   /* IB service number */
int  *kib_min_reconnect_interval; /* first failed connection
-  * retry... */
+  * retry...
+  */
int  *kib_max_reconnect_interval; /* ...exponentially increasing
-  * to this */
+  * to this
+  */
int  *kib_cksum; /* checksum kib_msg_t? */
int  *kib_timeout;   /* comms timeout (seconds) */
int  *kib_keepalive; /* keepalive timeout (seconds) */
@@ -92,9 +94,11 @@ typedef struct {
int  *kib_credits;   /* # concurrent sends */
int  *kib_peertxcredits; /* # concurrent sends to 1 peer */
int  *kib_peerrtrcredits;/* # per-peer router buffer
- * credits */
+ * credits
+ */
int  *kib_peercredits_hiw;   /* # when eagerly to return
- * credits */
+ * credits
+ */
int  *kib_peertimeout;   /* seconds to consider peer dead */
char **kib_default_ipif; /* default IPoIB interface */
int  *kib_retry_count;
@@ -103,13 +107,15 @@ typedef struct {
int  *kib_ib_mtu;/* IB MTU */
int  *kib_map_on_demand; /* map-on-demand if RD has more
  * fragments than this value, 0
- * disable map-on-demand */
+ * disable map-on-demand
+ */
int  *kib_fmr_pool_size; /* # FMRs in pool */
int  *kib_fmr_flush_trigger; /* When to trigger FMR flush */
int  *kib_fmr_cache; /* enable FMR pool cache? */
int  *kib_require_priv_port; /* accept only privileged ports */
int  *kib_use_priv_port; /* use privileged port for active
- * connect */
+ * connect
+ */
int  *kib_nscheds;   /* # threads on each CPT */
 } kib_tunables_t;
 
@@ -200,7 +206,8 @@ typedef struct {
intibd_failed_failover; /* # failover failures */
unsigned int   ibd_failover;/* failover in progress */
unsigned int   ibd_can_failover;/* IPoIB interface is a bonding
-* master */
+* master
+*/
struct list_head   ibd_nets;
struct kib_hca_dev *ibd_hdev;
 } kib_dev_t;
@@ -250,7 +257,8 @@ typedef struct kib_poolset {
struct list_head  ps_pool_list;   /* list of pools */
struct list_head  ps_failed_pool_list;/* failed pool list */
unsigned long ps_next_retry;  /* time stamp for retry if
-  * failed to allocate */
+  * failed to allocate
+  */
int   ps_increasing;  /* is allocating new pool */
int   ps_pool_size;   /* new pool size */
int   ps_cpt; /* CPT id */
@@ -258,7 +266,8 @@ typedef struct kib_poolset {
kib_ps_pool_create_t  ps_pool_create; /* create a new pool */
kib_ps_pool_destroy_t ps_pool_destroy;/* destroy a pool */
kib_ps_node_init_tps_node_init;   /* initialize new allocated
-  * 

RE: [lustre-devel] LIBCFS_ALLOC

[Simmons, James A.]

>> >Yeah.  You're right.  Doing a vmalloc() when kmalloc() doesn't have even
>> >a tiny sliver of RAM isn't going to work.  It's easier to use
>> >libcfs_kvzalloc() everywhere, but it's probably the wrong thing.
>>
>> The original  reason we have the vmalloc water mark wasn't so much the
>> issue of memory exhaustion but to handle the case of memory fragmentation.
>> Some sites had after a extended period of time started to see failures of
>> allocating even 32K using kmalloc.  In our latest development branch we moved
>> away from using a water mark to always try kmalloc first and if it fails 
>> then we
>> try vmalloc. At ORNL we ran into severe performance issues when we entered
>> vmalloc territory. It has been discussed before on what might replace vmalloc
>> handling in the case of kmalloc fails but no solution has been worked out.
>
>OK, but if a structure contains only 4 words, would it be better to just
>use kzalloc?  Or does it not matter?  It would only save trying vmalloc in
>a case that it is guaranteed to fail, but if a structure with 4 words
>can't be allocated, the system has other problems.  Another argument is
>that kzalloc is a well known function that people and bug-finding tools
>understand, so it is better to use it whenever possible.
>
>Some of the other structures contain a lot more fields, as well as small
>arrays.  They are probably acceptable for kzalloc too, but I wouldn't know
>the exact dividing line.

The reason I bring this up is to discuss sorting this out. Once long ago we had
just  LIBCFS_ALLOC. For some reason before my time OBD_ALLOC got spawned
off of that.  Currently LIBCFS_ALLOC is used just by the libcfs/LNet layer. Now 
OBD_ALLOC in our development branch has moved to a try kmalloc first and 
if it fails try vmalloc for any size memory allocation.  LIBCFS_ALLOC still 
does the
original approach.   So we have two possible solutions depending on if 
libcfs/LNet
needs to ever do a vmalloc.

One solution if libcfs/LNet never needs a vmalloc is remove LIBCFS_ALLOC and 
replace
it with kzalloc everywhere. We can then move libcfs_kzvalloc to the lustre 
layer and
port the change we did in the development branch to here of the try kmalloc then
vmalloc approach. 

The other approach is if libcfs/LNet does in some case need to use vmalloc we 
could 
then update LIBCFS_ALLOC to first try kmalloc then vmalloc. Once this is 
implemented
we can nuke the OBD_ALLOC system.

Either way I like to see it consolidated down to one system.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [lustre-devel] LIBCFS_ALLOC

[Simmons, James A.]

 Yeah.  You're right.  Doing a vmalloc() when kmalloc() doesn't have even
 a tiny sliver of RAM isn't going to work.  It's easier to use
 libcfs_kvzalloc() everywhere, but it's probably the wrong thing.

 The original  reason we have the vmalloc water mark wasn't so much the
 issue of memory exhaustion but to handle the case of memory fragmentation.
 Some sites had after a extended period of time started to see failures of
 allocating even 32K using kmalloc.  In our latest development branch we moved
 away from using a water mark to always try kmalloc first and if it fails 
 then we
 try vmalloc. At ORNL we ran into severe performance issues when we entered
 vmalloc territory. It has been discussed before on what might replace vmalloc
 handling in the case of kmalloc fails but no solution has been worked out.

OK, but if a structure contains only 4 words, would it be better to just
use kzalloc?  Or does it not matter?  It would only save trying vmalloc in
a case that it is guaranteed to fail, but if a structure with 4 words
can't be allocated, the system has other problems.  Another argument is
that kzalloc is a well known function that people and bug-finding tools
understand, so it is better to use it whenever possible.

Some of the other structures contain a lot more fields, as well as small
arrays.  They are probably acceptable for kzalloc too, but I wouldn't know
the exact dividing line.

The reason I bring this up is to discuss sorting this out. Once long ago we had
just  LIBCFS_ALLOC. For some reason before my time OBD_ALLOC got spawned
off of that.  Currently LIBCFS_ALLOC is used just by the libcfs/LNet layer. Now 
OBD_ALLOC in our development branch has moved to a try kmalloc first and 
if it fails try vmalloc for any size memory allocation.  LIBCFS_ALLOC still 
does the
original approach.   So we have two possible solutions depending on if 
libcfs/LNet
needs to ever do a vmalloc.

One solution if libcfs/LNet never needs a vmalloc is remove LIBCFS_ALLOC and 
replace
it with kzalloc everywhere. We can then move libcfs_kzvalloc to the lustre 
layer and
port the change we did in the development branch to here of the try kmalloc then
vmalloc approach. 

The other approach is if libcfs/LNet does in some case need to use vmalloc we 
could 
then update LIBCFS_ALLOC to first try kmalloc then vmalloc. Once this is 
implemented
we can nuke the OBD_ALLOC system.

Either way I like to see it consolidated down to one system.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: LIBCFS_ALLOC

[Simmons, James A.]

>Yeah.  You're right.  Doing a vmalloc() when kmalloc() doesn't have even
>a tiny sliver of RAM isn't going to work.  It's easier to use
>libcfs_kvzalloc() everywhere, but it's probably the wrong thing.

The original  reason we have the vmalloc water mark wasn't so much the
issue of memory exhaustion but to handle the case of memory fragmentation. 
Some sites had after a extended period of time started to see failures of
allocating even 32K using kmalloc.  In our latest development branch we moved
away from using a water mark to always try kmalloc first and if it fails then we
try vmalloc. At ORNL we ran into severe performance issues when we entered
vmalloc territory. It has been discussed before on what might replace vmalloc
handling in the case of kmalloc fails but no solution has been worked out.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: LIBCFS_ALLOC

[Simmons, James A.]

Yeah.  You're right.  Doing a vmalloc() when kmalloc() doesn't have even
a tiny sliver of RAM isn't going to work.  It's easier to use
libcfs_kvzalloc() everywhere, but it's probably the wrong thing.

The original  reason we have the vmalloc water mark wasn't so much the
issue of memory exhaustion but to handle the case of memory fragmentation. 
Some sites had after a extended period of time started to see failures of
allocating even 32K using kmalloc.  In our latest development branch we moved
away from using a water mark to always try kmalloc first and if it fails then we
try vmalloc. At ORNL we ran into severe performance issues when we entered
vmalloc territory. It has been discussed before on what might replace vmalloc
handling in the case of kmalloc fails but no solution has been worked out.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH v4 4/9] staging:lustre: merge socklnd_lib-linux.h into socklnd.h

[Simmons, James A.]

>> > diff --git a/drivers/staging/lustre/lnet/klnds/socklnd/socklnd.h 
>> > b/drivers/staging/lustre/lnet/klnds/socklnd/socklnd.h
>> > index 53275f9..7125eb9 100644
>> > --- a/drivers/staging/lustre/lnet/klnds/socklnd/socklnd.h
>> > +++ b/drivers/staging/lustre/lnet/klnds/socklnd/socklnd.h
>> > @@ -25,16 +25,40 @@
>> >   *
>> >   */
>> >
>> > +#ifndef _SOCKLND_SOCKLND_H_
>> > +#define _SOCKLND_SOCKLND_H_
>> > +
>> >  #define DEBUG_PORTAL_ALLOC
>> >  #define DEBUG_SUBSYSTEM S_LND
>> >
>> > -#include "socklnd_lib-linux.h"
>> > +#include 
>> > +#include 
>> > +#include 
>> 
>> Including  first causes a build failure for m68k/allmodconfig:
>> 
>> arch/m68k/include/asm/irq.h:77:12: error: expected '=', ',', ';',
>> 'asm' or '__attribute__' before 'void'
>> arch/m68k/include/asm/irq.h:78:1: error: unknown type name 'atomic_t'
>> arch/m68k/include/asm/irq.h:77:12: error: expected '=', ',', ';',
>> 'asm' or '__attribute__' before 'void'
>> arch/m68k/include/asm/irq.h:78:1: error: unknown type name 'atomic_t'
>> 
>> http://kisskb.ellerman.id.au/kisskb/buildresult/12448922/
>> 
>> Fixing it inside arch/m68k/include/asm/irq.h might cause Include Hell,
>> so perhaps you can just move the  include below all 
>> includes?

I looked at our main development branch and I see socklnd.h no longer has
#include . We can just remove the irq.h from socklnd.h.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH v4 4/9] staging:lustre: merge socklnd_lib-linux.h into socklnd.h

[Simmons, James A.]

>On Thu, Jun 25, 2015 at 3:33 AM, Guenter Roeck  wrote:
>> I have not tested it, but I think the following may fix the problem
>> while avoiding any include problems. Since pt_regs is used in the file,
>> one could argue that it should be declared.
>
>Indeed. I tried that, but...
>
>> --
>> diff --git a/arch/m68k/include/asm/irq.h b/arch/m68k/include/asm/irq.h
>> index 81ca118d58af..28ffa8d59cf0 100644
>> --- a/arch/m68k/include/asm/irq.h
>> +++ b/arch/m68k/include/asm/irq.h
>> @@ -74,6 +74,8 @@ extern unsigned int irq_canonicalize(unsigned int irq);
>>  #define irq_canonicalize(irq)  (irq)
>>  #endif /* !(CONFIG_M68020 || CONFIG_M68030 || CONFIG_M68040 || 
>> CONFIG_M68060) */
>>
>> +struct pt_regs;
>> +
>>  asmlinkage void do_IRQ(int irq, struct pt_regs *regs);
>>  extern atomic_t irq_err_count;
>
>... asmlinkage and atomic_t are also needed.
>
>I didn't want to risk introducing more breakage by adding (at least) three
>more includes.

Hi Geert

  Long time. I agree the above is not the best approach. Lets fix lustre 
instead.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH v4 4/9] staging:lustre: merge socklnd_lib-linux.h into socklnd.h

[Simmons, James A.]

On Thu, Jun 25, 2015 at 3:33 AM, Guenter Roeck li...@roeck-us.net wrote:
 I have not tested it, but I think the following may fix the problem
 while avoiding any include problems. Since pt_regs is used in the file,
 one could argue that it should be declared.

Indeed. I tried that, but...

 --
 diff --git a/arch/m68k/include/asm/irq.h b/arch/m68k/include/asm/irq.h
 index 81ca118d58af..28ffa8d59cf0 100644
 --- a/arch/m68k/include/asm/irq.h
 +++ b/arch/m68k/include/asm/irq.h
 @@ -74,6 +74,8 @@ extern unsigned int irq_canonicalize(unsigned int irq);
  #define irq_canonicalize(irq)  (irq)
  #endif /* !(CONFIG_M68020 || CONFIG_M68030 || CONFIG_M68040 || 
 CONFIG_M68060) */

 +struct pt_regs;
 +
  asmlinkage void do_IRQ(int irq, struct pt_regs *regs);
  extern atomic_t irq_err_count;

... asmlinkage and atomic_t are also needed.

I didn't want to risk introducing more breakage by adding (at least) three
more includes.

Hi Geert

  Long time. I agree the above is not the best approach. Lets fix lustre 
instead.

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH v4 4/9] staging:lustre: merge socklnd_lib-linux.h into socklnd.h

[Simmons, James A.]

  diff --git a/drivers/staging/lustre/lnet/klnds/socklnd/socklnd.h 
  b/drivers/staging/lustre/lnet/klnds/socklnd/socklnd.h
  index 53275f9..7125eb9 100644
  --- a/drivers/staging/lustre/lnet/klnds/socklnd/socklnd.h
  +++ b/drivers/staging/lustre/lnet/klnds/socklnd/socklnd.h
  @@ -25,16 +25,40 @@
*
*/
 
  +#ifndef _SOCKLND_SOCKLND_H_
  +#define _SOCKLND_SOCKLND_H_
  +
   #define DEBUG_PORTAL_ALLOC
   #define DEBUG_SUBSYSTEM S_LND
 
  -#include socklnd_lib-linux.h
  +#include asm/irq.h
  +#include linux/crc32.h
  +#include linux/errno.h
 
 Including asm/irq.h first causes a build failure for m68k/allmodconfig:
 
 arch/m68k/include/asm/irq.h:77:12: error: expected '=', ',', ';',
 'asm' or '__attribute__' before 'void'
 arch/m68k/include/asm/irq.h:78:1: error: unknown type name 'atomic_t'
 arch/m68k/include/asm/irq.h:77:12: error: expected '=', ',', ';',
 'asm' or '__attribute__' before 'void'
 arch/m68k/include/asm/irq.h:78:1: error: unknown type name 'atomic_t'
 
 http://kisskb.ellerman.id.au/kisskb/buildresult/12448922/
 
 Fixing it inside arch/m68k/include/asm/irq.h might cause Include Hell,
 so perhaps you can just move the asm/* include below all linux/*
 includes?

I looked at our main development branch and I see socklnd.h no longer has
#include asm/irq.h. We can just remove the irq.h from socklnd.h.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [lustre-devel] [PATCH 01/12] staging: lustre: fid: Use !x to check for kzalloc failure

[Simmons, James A.]

>> Yes.  I know Al's thoughts and kernel style.
>>
>> But Alan Cox and Andreas have both said they think (x == NULL) can help
>> you avoid some kind of boolean vs pointer bugs.  I've had co-workers who
>> did massive seds changing !foo to foo == NULL on our code base.  But
>> I've never seen a real life example of a bug this fixes.
>>
>> To be honest, I've never seen a real life proof that (!foo) code is less
>> buggy.  I should look through the kbuild mailbox...  Hm...  But my other
>> idea of setting up code style readability testing website is also a good
>> one.
>>
>> Linux kernel style is based on Joe Perches finding that 80% of the code
>> prefers one way or the other.  That's a valid method for determining
>> code style.  I bet it normally picks the more readable style but it
>> would be interesting to measure it more formally.
>
>On today's linux-next, I find 3218 tests on the result of kmalloc etc
>using NULL and 14429 without, making 82% without.  The complete semantic
>patch is shown below.

Most people doing something a certain way is not a technical argument. Usually
people do what they are taught. From most people's comments their seems to 
be no technical reason to us one over another. I do have one technical reason 
not
to accept these patches. It is too easy to make a mistake and break things very 
badly.
I don't think it is worth the risk for a non-hard requirement.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [lustre-devel] [PATCH 01/12] staging: lustre: fid: Use !x to check for kzalloc failure

[Simmons, James A.]

 Yes.  I know Al's thoughts and kernel style.

 But Alan Cox and Andreas have both said they think (x == NULL) can help
 you avoid some kind of boolean vs pointer bugs.  I've had co-workers who
 did massive seds changing !foo to foo == NULL on our code base.  But
 I've never seen a real life example of a bug this fixes.

 To be honest, I've never seen a real life proof that (!foo) code is less
 buggy.  I should look through the kbuild mailbox...  Hm...  But my other
 idea of setting up code style readability testing website is also a good
 one.

 Linux kernel style is based on Joe Perches finding that 80% of the code
 prefers one way or the other.  That's a valid method for determining
 code style.  I bet it normally picks the more readable style but it
 would be interesting to measure it more formally.

On today's linux-next, I find 3218 tests on the result of kmalloc etc
using NULL and 14429 without, making 82% without.  The complete semantic
patch is shown below.

Most people doing something a certain way is not a technical argument. Usually
people do what they are taught. From most people's comments their seems to 
be no technical reason to us one over another. I do have one technical reason 
not
to accept these patches. It is too easy to make a mistake and break things very 
badly.
I don't think it is worth the risk for a non-hard requirement.

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [HPDD-discuss] [PATCH 1/1] staging: lustre/lustre/llite: get rid of incorrect type warning

[Simmons, James A.]

>>On Wed, Jun 10, 2015 at 5:48 PM, Greg Kroah-Hartman
> wrote:
>>
>> Are you sure all of these are correct?  The kernel/user api for lustre
>> is a complex beast, and just casting away the pointer types isn't
>> usually the proper thing to do in order to resolve the issues here.
>>
>> thanks,
>>
>> greg k-h
>
>I'm not 100% sure, but the pointers that I added the annotation to end
>up being used as user memory. (eg. passed to copy_to_user, etc.)
>Sometimes these pointers are passed to functions that already have
>__user annotation in their signatures (eg. ll_getname, copy_and_ioctl,
>ll_fid2path, etc.).

Using these simple cast are not the proper fix. We had a lot of issues with user
land tools breaking due to leakage of kernel space stuff and other problems.
Some work went into cleaning that up in the OpenSFS branch but it is not totally
complete yet. Evans you wanted something challenging to work on well this is
up your alley. I would recommend looking at JIRA ticket LU-6401 and all its sub
tickets.  You could start the port of those to the upstream client. At the same
time we can finish the cleanup in the OpenSFS branch as well.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [HPDD-discuss] [PATCH 1/1] staging: lustre/lustre/llite: get rid of incorrect type warning

[Simmons, James A.]

On Wed, Jun 10, 2015 at 5:48 PM, Greg Kroah-Hartman
gre...@linuxfoundation.org wrote:

 Are you sure all of these are correct?  The kernel/user api for lustre
 is a complex beast, and just casting away the pointer types isn't
 usually the proper thing to do in order to resolve the issues here.

 thanks,

 greg k-h

I'm not 100% sure, but the pointers that I added the annotation to end
up being used as user memory. (eg. passed to copy_to_user, etc.)
Sometimes these pointers are passed to functions that already have
__user annotation in their signatures (eg. ll_getname, copy_and_ioctl,
ll_fid2path, etc.).

Using these simple cast are not the proper fix. We had a lot of issues with user
land tools breaking due to leakage of kernel space stuff and other problems.
Some work went into cleaning that up in the OpenSFS branch but it is not totally
complete yet. Evans you wanted something challenging to work on well this is
up your alley. I would recommend looking at JIRA ticket LU-6401 and all its sub
tickets.  You could start the port of those to the upstream client. At the same
time we can finish the cleanup in the OpenSFS branch as well.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [lustre-devel] [PATCH v3 7/8] staging:lustre: style cleanups for LNet headers

[Simmons, James A.]

>We're going through and re-indenting things.  I think just one space
>between type and name is the right thing for .c files but you guys
>really should figure out what style you're using for your header files.

This is good to know. I didn't know how you felt about the lustre style.
I will fix it up in the next patch.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [lustre-devel] [PATCH v3 5/8] staging:lustre: separate kernel and user land defines in the LNet headers

[Simmons, James A.]

>On 2015/06/05, 3:02 AM, "Dan Carpenter"  wrote:
>
>>On Wed, Jun 03, 2015 at 04:43:24PM -0400, James Simmons wrote:
>>> Currently the lnet headers used by user land contain various internal
>>> LNet structures that are only used by kernel space. Move the user land
>>> structures to headers used by user land. The kernel structures are
>>> relocated to headers that are never exposed to user land.
>>> 
>>> Signed-off-by: James Simmons 
>>> ---
>>> diff --git a/drivers/staging/lustre/lnet/lnet/acceptor.c
>>>b/drivers/staging/lustre/lnet/lnet/acceptor.c
>>> index 1dc7c8a..4928f5c 100644
>>> --- a/drivers/staging/lustre/lnet/lnet/acceptor.c
>>> +++ b/drivers/staging/lustre/lnet/lnet/acceptor.c
>>> @@ -243,8 +243,6 @@ lnet_accept(struct socket *sock, __u32 magic)
>>>  
>>> if (magic == le32_to_cpu(LNET_PROTO_TCP_MAGIC))
>>> str = "'old' socknal/tcpnal";
>>> -   else if (lnet_accept_magic(magic, LNET_PROTO_RA_MAGIC))
>>> -   str = "'old' ranal";
>>> else
>>> str = "unrecognised";
>>>  
>>
>>Presumably this was done intentionally.  We deleted LNET_PROTO_RA_MAGIC.
>>The changelog was not very clear why.
>
>The "Rapid Array" network interface is an obsolete network once used by
>Cray hardware but hasn't been supported for about 10 years and was (mostly)
>deleted from the tree already.  This was just a left-over I guess.

I need to break this patch up and do a proper backport of the patch from 
LU-6209.
Currently it is mixed in with this header cleanup. 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/