Re: [[PATCH v1] 02/37] [CIFS] SMBD: Add structure for SMBD transport

classic Classic list List threaded Threaded
6 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [[PATCH v1] 02/37] [CIFS] SMBD: Add structure for SMBD transport

Samba - samba-technical mailing list
Hi Li,

thanks for providing this patchset, I guess it will be a huge win to
have SMBDirect support for the kernel client!

> Define a new structure for SMBD transport. This stucture will have all the
> information on the transport, and it will be stored in the current SMB session.
...
> +/*
> + * The context for the SMBDirect transport
> + * Everything related to the transport is here. It has several
logical parts

> + * 1. RDMA related structures
> + * 2. SMBDirect connection parameters
> + * 3. Reassembly queue for data receive path
> + * 4. mempools for allocating packets
> + */
> +struct cifs_rdma_info {
> + struct TCP_Server_Info *server_info;
> +
> + // for debug purposes
> + unsigned int count_receive_buffer;
> + unsigned int count_get_receive_buffer;
> + unsigned int count_put_receive_buffer;
> + unsigned int count_send_empty;
> +};
> +#endif
>

It seems that the new transport is tied to it's caller
regarding structures and naming conventions.

I think it would be better to strictly separate them,
as I'd like to use the SMBDirect transport also from the
userspace for the client side e.g. in Samba's '[lib]smbclient',
but also in Samba's server side code 'smbd'.

Would it be possible to isolate this in
smb_direct.c and smb_direct.h while using
smb_direct_* prefixes for structures and
functions? Also avoiding the usage of other headers
from fs/cifs/*.h, expect for something generic like
nterr.h.

I guess 'struct cifs_rdma_info' would then be
'struct smb_direct_connection'. And it won't
have a reference to struct TCP_Server_Info.

It the strict layering is too much change,
I'd at least like to have the name changes.

This should relatively easy to do by using somthing like

git format-patch --stdout -37 > before

cat before | sed \
-e 's!struct cifs_rdma_info!struct smb_direct_connection!g' \
-e 's!cifsrdma\.h!smb_direct.h!g' \
-e 's!cifsrdma\.c!smb_direct.c!g' \
> after

git reset --hard HEAD~37
git am after

metze

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

RE: [[PATCH v1] 02/37] [CIFS] SMBD: Add structure for SMBD transport

Samba - samba-technical mailing list
> -----Original Message-----
> From: Stefan Metzmacher [mailto:[hidden email]]
> Sent: Monday, August 7, 2017 11:58 PM
> To: Steve French <[hidden email]>; [hidden email]; samba-
> [hidden email]; [hidden email]
> Cc: Long Li <[hidden email]>
> Subject: Re: [[PATCH v1] 02/37] [CIFS] SMBD: Add structure for SMBD
> transport
>
> Hi Li,
>
> thanks for providing this patchset, I guess it will be a huge win to have
> SMBDirect support for the kernel client!
>
> > Define a new structure for SMBD transport. This stucture will have all
> > the information on the transport, and it will be stored in the current SMB
> session.
> ...
> > +/*
> > + * The context for the SMBDirect transport
> > + * Everything related to the transport is here. It has several
> logical parts
> > + * 1. RDMA related structures
> > + * 2. SMBDirect connection parameters
> > + * 3. Reassembly queue for data receive path
> > + * 4. mempools for allocating packets  */ struct cifs_rdma_info {
> > + struct TCP_Server_Info *server_info;
> > +
> > + // for debug purposes
> > + unsigned int count_receive_buffer;
> > + unsigned int count_get_receive_buffer;
> > + unsigned int count_put_receive_buffer;
> > + unsigned int count_send_empty;
> > +};
> > +#endif
> >
>
> It seems that the new transport is tied to it's caller regarding structures and
> naming conventions.
>
> I think it would be better to strictly separate them, as I'd like to use the
> SMBDirect transport also from the userspace for the client side e.g. in
> Samba's '[lib]smbclient', but also in Samba's server side code 'smbd'.

Thank you for reviewing the patch set.

I think it is possible to separate the common code that implements the SMBDirect transport. There are some challenges to reuse the same code for both kernel and user spaces.
1. Kernel mode RDMA verbs are similar but different to user-mode ones.
2. Some RDMA features (e.g Fast Registration Work Request) are not available in user-mode.
3. Locking and synchronization mechanism is different
4. Memory management is different.
5. Process creation/scheduling and data sharing between processes are different, and there is no user-mode code running in interrupt/softirq.

Those needs to be abstracted through a layer, the rest of the code can be shared. I can work on this after patch set is reviewed.

>
> Would it be possible to isolate this in
> smb_direct.c and smb_direct.h while using
> smb_direct_* prefixes for structures and functions? Also avoiding the usage
> of other headers from fs/cifs/*.h, expect for something generic like nterr.h.

Sure I will make naming changes and clean up the header files.
>
> I guess 'struct cifs_rdma_info' would then be 'struct smb_direct_connection'.
> And it won't have a reference to struct TCP_Server_Info.

I will look for ways to remove reference to struct TCP_Server_Info . The reason why it has a reference to TCP_Server_Info is that: TCP_Server_Info represents a transport connection, although it also has many other TCP related code. SMBD needs to get to this connection TCP_Server_Info and set the transport status on shutdown (and maybe other situations).


Long

>
> It the strict layering is too much change, I'd at least like to have the name
> changes.
>
> This should relatively easy to do by using somthing like
>
> git format-patch --stdout -37 > before
>
> cat before | sed \
> -e 's!struct cifs_rdma_info!struct smb_direct_connection!g' \ -e
> 's!cifsrdma\.h!smb_direct.h!g' \ -e 's!cifsrdma\.c!smb_direct.c!g' \
> > after
>
> git reset --hard HEAD~37
> git am after
>
> metze
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [[PATCH v1] 02/37] [CIFS] SMBD: Add structure for SMBD transport

Samba - samba-technical mailing list
On Sat, Aug 12, 2017 at 08:32:48AM +0000, Long Li via samba-technical wrote:
> I think it is possible to separate the common code that implements the SMBDirect transport. There are some challenges to reuse the same code for both kernel and user spaces.
> 1. Kernel mode RDMA verbs are similar but different to user-mode ones.
> 2. Some RDMA features (e.g Fast Registration Work Request) are not available in user-mode.
> 3. Locking and synchronization mechanism is different
> 4. Memory management is different.
> 5. Process creation/scheduling and data sharing between processes are different, and there is no user-mode code running in interrupt/softirq.
>
> Those needs to be abstracted through a layer, the rest of the code can be shared. I can work on this after patch set is reviewed.

NAK - code with those sort of obsfucation layer will be rejected
for kernel inclusion - don't add it.

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [[PATCH v1] 02/37] [CIFS] SMBD: Add structure for SMBD transport

Samba - samba-technical mailing list
In reply to this post by Samba - samba-technical mailing list
On Wed, Aug 02, 2017 at 01:10:13PM -0700, Long Li wrote:

> From: Long Li <[hidden email]>
>
> Define a new structure for SMBD transport. This stucture will have all the
> information on the transport, and it will be stored in the current SMB session.
>
> Signed-off-by: Long Li <[hidden email]>
> ---
>  fs/cifs/cifsrdma.c | 56 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  fs/cifs/cifsrdma.h | 45 +++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 101 insertions(+)
>  create mode 100644 fs/cifs/cifsrdma.c
>  create mode 100644 fs/cifs/cifsrdma.h
>
> diff --git a/fs/cifs/cifsrdma.c b/fs/cifs/cifsrdma.c
> new file mode 100644
> index 0000000..a2c0478
> --- /dev/null
> +++ b/fs/cifs/cifsrdma.c
> @@ -0,0 +1,56 @@
> +/*
> + *   Copyright (C) 2017, Microsoft Corporation.
> + *
> + *   Author(s): Long Li <[hidden email]>
> + *
> + *   This program is free software;  you can redistribute it and/or modify
> + *   it under the terms of the GNU General Public License as published by
> + *   the Free Software Foundation; either version 2 of the License, or
> + *   (at your option) any later version.
> + *
> + *   This program is distributed in the hope that it will be useful,
> + *   but WITHOUT ANY WARRANTY;  without even the implied warranty of
> + *   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See
> + *   the GNU General Public License for more details.
> + *
> + *   You should have received a copy of the GNU General Public License
> + *   along with this program;  if not, write to the Free Software
> + *   Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
> + */
> +#include <linux/fs.h>
> +#include <linux/net.h>
> +#include <linux/string.h>
> +#include <linux/list.h>
> +#include <linux/wait.h>
> +#include <linux/slab.h>
> +#include <linux/pagemap.h>
> +#include <linux/ctype.h>
> +#include <linux/utsname.h>
> +#include <linux/mempool.h>
> +#include <linux/delay.h>
> +#include <linux/completion.h>
> +#include <linux/kthread.h>
> +#include <linux/pagevec.h>
> +#include <linux/freezer.h>
> +#include <linux/namei.h>
> +#include <asm/uaccess.h>
> +#include <asm/processor.h>
> +#include <linux/inet.h>
> +#include <linux/module.h>
> +#include <keys/user-type.h>
> +#include <net/ipv6.h>
> +#include <linux/parser.h>

Where do all these includes come from?  It seems like most of them
are not actually used in the code.

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [[PATCH v1] 02/37] [CIFS] SMBD: Add structure for SMBD transport

Samba - samba-technical mailing list
In reply to this post by Samba - samba-technical mailing list
Hi Long,

>> It seems that the new transport is tied to it's caller regarding structures and
>> naming conventions.
>>
>> I think it would be better to strictly separate them, as I'd like to use the
>> SMBDirect transport also from the userspace for the client side e.g. in
>> Samba's '[lib]smbclient', but also in Samba's server side code 'smbd'.
>
> Thank you for reviewing the patch set.
>
> I think it is possible to separate the common code that implements the SMBDirect transport. There are some challenges to reuse the same code for both kernel and user spaces.
> 1. Kernel mode RDMA verbs are similar but different to user-mode ones.
> 2. Some RDMA features (e.g Fast Registration Work Request) are not available in user-mode.
> 3. Locking and synchronization mechanism is different
> 4. Memory management is different.
> 5. Process creation/scheduling and data sharing between processes are different, and there is no user-mode code running in interrupt/softirq.
>
> Those needs to be abstracted through a layer, the rest of the code can be shared. I can work on this after patch set is reviewed.
I guess this is a misunderstanding...

I don't want to use that code and run it in userspace,
I have a userspace prototype more or less working here, see
https://git.samba.org/?p=metze/samba/wip.git;a=shortlog;h=refs/heads/master3-rdma
and
https://git.samba.org/?p=metze/samba/wip.git;a=blob;f=libcli/smb/smb_direct.c;h=9cc0d861ccfcbb4df9ef6ad85a7fe3d262e549c0;hb=85d46de6fdbba041d3e8004af46865a72d2b8405

I goal is that we'll have an api that allows userspace
code to use the kernel code SMBDirect code. This
userspace code would get a file descriptor from the kernel
and would be able to use it similar to a tcp socket.
If the kernel would simulate the 4 byte length header,
it's trivial to get to a stage were smbclient and smbd
are able to support SMBDirect without much changes.
We only need to replace connect(), listen(), accept() and a few more
by SMBDirect versions.

For the real data transfer we might be able to use memfd_create()
or something similar to share the buffers between userspace and kernel.

I guess this is a long way, but having the basic SMBDirect code in
dependently in the kernel would help a lot.

>> Would it be possible to isolate this in
>> smb_direct.c and smb_direct.h while using
>> smb_direct_* prefixes for structures and functions? Also avoiding the usage
>> of other headers from fs/cifs/*.h, expect for something generic like nterr.h.
>
> Sure I will make naming changes and clean up the header files.

Thanks!

>> I guess 'struct cifs_rdma_info' would then be 'struct smb_direct_connection'.
>> And it won't have a reference to struct TCP_Server_Info.
>
> I will look for ways to remove reference to struct TCP_Server_Info . The reason why it has a reference to TCP_Server_Info is that: TCP_Server_Info represents a transport connection, although it also has many other TCP related code. SMBD needs to get to this connection TCP_Server_Info and set the transport status on shutdown (and maybe other situations).
>

Wouldn't it be better to provide a way to ask for the connection state
and let the caller ask for the state instead of changing the callers
structure?

metze


signature.asc (853 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

RE: [[PATCH v1] 02/37] [CIFS] SMBD: Add structure for SMBD transport

Samba - samba-technical mailing list


> -----Original Message-----
> From: Stefan Metzmacher [mailto:[hidden email]]
> Sent: Monday, August 14, 2017 6:41 AM
> To: Long Li <[hidden email]>; Steve French <[hidden email]>;
> [hidden email]; [hidden email]; linux-
> [hidden email]; [hidden email]; Christoph Hellwig
> <[hidden email]>
> Subject: Re: [[PATCH v1] 02/37] [CIFS] SMBD: Add structure for SMBD
> transport
>
> Hi Long,
>
> >> It seems that the new transport is tied to it's caller regarding
> >> structures and naming conventions.
> >>
> >> I think it would be better to strictly separate them, as I'd like to
> >> use the SMBDirect transport also from the userspace for the client
> >> side e.g. in Samba's '[lib]smbclient', but also in Samba's server side code
> 'smbd'.
> >
> > Thank you for reviewing the patch set.
> >
> > I think it is possible to separate the common code that implements the
> SMBDirect transport. There are some challenges to reuse the same code for
> both kernel and user spaces.
> > 1. Kernel mode RDMA verbs are similar but different to user-mode ones.
> > 2. Some RDMA features (e.g Fast Registration Work Request) are not
> available in user-mode.
> > 3. Locking and synchronization mechanism is different 4. Memory
> > management is different.
> > 5. Process creation/scheduling and data sharing between processes are
> different, and there is no user-mode code running in interrupt/softirq.
> >
> > Those needs to be abstracted through a layer, the rest of the code can be
> shared. I can work on this after patch set is reviewed.
>
> I guess this is a misunderstanding...
>
> I don't want to use that code and run it in userspace, I have a userspace
> prototype more or less working here, see
> https://git.samba.org/?p=metze/samba/wip.git;a=shortlog;h=refs/heads/m
> aster3-rdma
> and
> https://git.samba.org/?p=metze/samba/wip.git;a=blob;f=libcli/smb/smb_dir
> ect.c;h=9cc0d861ccfcbb4df9ef6ad85a7fe3d262e549c0;hb=85d46de6fdbba041
> d3e8004af46865a72d2b8405
>
> I goal is that we'll have an api that allows userspace code to use the kernel
> code SMBDirect code. This userspace code would get a file descriptor from
> the kernel and would be able to use it similar to a tcp socket.
> If the kernel would simulate the 4 byte length header, it's trivial to get to a
> stage were smbclient and smbd are able to support SMBDirect without much
> changes.
> We only need to replace connect(), listen(), accept() and a few more by
> SMBDirect versions.

This is possible.  SMBDirect code can handle the first 4 bytes for upper layer.
>
> For the real data transfer we might be able to use memfd_create() or
> something similar to share the buffers between userspace and kernel.

You'll need to post RDMA read/write through the same QP created by SMBDirect in the kernel. I think this needs some more work but it's doable.

>
> I guess this is a long way, but having the basic SMBDirect code in dependently
> in the kernel would help a lot.
>
> >> Would it be possible to isolate this in smb_direct.c and smb_direct.h
> >> while using
> >> smb_direct_* prefixes for structures and functions? Also avoiding the
> >> usage of other headers from fs/cifs/*.h, expect for something generic like
> nterr.h.
> >
> > Sure I will make naming changes and clean up the header files.
>
> Thanks!
>
> >> I guess 'struct cifs_rdma_info' would then be 'struct
> smb_direct_connection'.
> >> And it won't have a reference to struct TCP_Server_Info.
> >
> > I will look for ways to remove reference to struct TCP_Server_Info . The
> reason why it has a reference to TCP_Server_Info is that: TCP_Server_Info
> represents a transport connection, although it also has many other TCP
> related code. SMBD needs to get to this connection TCP_Server_Info and set
> the transport status on shutdown (and maybe other situations).
> >
>
> Wouldn't it be better to provide a way to ask for the connection state and let
> the caller ask for the state instead of changing the callers structure?
>
> metze

Loading...