| Message ID | 1501704648-20159-3-git-send-email-longli@exchange.microsoft.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 -- To unsubscribe from this list: send the line "unsubscribe linux-cifs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> -----Original Message----- > From: Stefan Metzmacher [mailto:metze@samba.org] > Sent: Monday, August 7, 2017 11:58 PM > To: Steve French <sfrench@samba.org>; linux-cifs@vger.kernel.org; samba- > technical@lists.samba.org; linux-kernel@vger.kernel.org > Cc: Long Li <longli@microsoft.com> > 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
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. -- To unsubscribe from this list: send the line "unsubscribe linux-cifs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Aug 02, 2017 at 01:10:13PM -0700, Long Li wrote: > From: Long Li <longli@microsoft.com> > > 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 <longli@microsoft.com> > --- > 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 <longli@microsoft.com> > + * > + * 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. -- To unsubscribe from this list: send the line "unsubscribe linux-cifs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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
> -----Original Message----- > From: Stefan Metzmacher [mailto:metze@samba.org] > Sent: Monday, August 14, 2017 6:41 AM > To: Long Li <longli@microsoft.com>; Steve French <sfrench@samba.org>; > linux-cifs@vger.kernel.org; samba-technical@lists.samba.org; linux- > kernel@vger.kernel.org; linux-rdma@vger.kernel.org; Christoph Hellwig > <hch@infradead.org> > 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
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 <longli@microsoft.com> + * + * 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> + +#include "cifspdu.h" +#include "cifsglob.h" +#include "cifsproto.h" +#include "cifs_unicode.h" +#include "cifs_debug.h" +#include "cifs_fs_sb.h" +#include "ntlmssp.h" +#include "nterr.h" +#include "rfc1002pdu.h" +#include "fscache.h" + +#include "cifsrdma.h" + diff --git a/fs/cifs/cifsrdma.h b/fs/cifs/cifsrdma.h new file mode 100644 index 0000000..ec6aa61 --- /dev/null +++ b/fs/cifs/cifsrdma.h @@ -0,0 +1,45 @@ +/* + * Copyright (C) 2017, Microsoft Corporation. + * + * Author(s): Long Li <longli@microsoft.com> + * + * 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 + */ +#ifndef _CIFS_RDMA_H +#define _CIFS_RDMA_H + +#include "cifsglob.h" +#include <rdma/ib_verbs.h> +#include <rdma/rdma_cm.h> +#include <linux/mempool.h> + +/* + * 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