Hi Rakesh,

Thanks for starting on this, I agree with you that for simplicity that
the uIP daemon should be integrated into the iscsid daemon.  My comments
are below:

On Mon, 2009-06-22 at 05:59 -0700, Rakesh Ranjan wrote:
> Hi Guys,
> 
> Following are my suggestions to get rid of vendor provided daemon for
> SNIC configuration and use iscsid for DHCP configuration and
> provisioning.
> 
> 1. Remove ISCSID_UIP_IPC_GET_IFACE IPC message and related handlers.

You are correct, this is not necessary because the uIP application is
going away.

> 2. Use set_net_config callback to configure SNIC using vendor provided
> library.
> 3. Do the necessary code changes in various module to integrate the
> whole functionality without need of any extra user space daemon.
> 4. Get rid of ISCSI_NL_GRP_UIP netlink message and use
> ISCSI_NL_GRP_ISCSID instead. Move the netlink processing in nic_nl.c
> into netlink.

Correct, it is safe to remove ISCSI_NL_GRP_UIP because we could then
rely on ISCSI_NL_GRP_ISCSID instead.  netlink.c::ctldev_handle() would
need to be modified to handle 'ISCSI_KEVENT_PATH_REQ' and
'ISCSI_KEVENT_IF_DOWN'

> 
> I have done some initial modification in my private tree and I am
> attaching whole tree. It won't even compile for now since related build
> infra is missing. First I would like to have feedback/suggestions from
> you guys before going ahead.

Could you also send all the .git directory containing all the metadata?
This way I can see all the commits that you have made to your tree?

> 
> 
> A. I have modified iface_rec to include vendor library path, UIO dev
> node path.
> 
> typedef struct iface_rec {
>           struct list_head        list;
>           /* iscsi iface record name */
>           char                    name[ISCSI_MAX_IFACE_LEN];
>           /* network layer iface name (eth0) */
>           char                    netdev[IFNAMSIZ];
>           char                    ipaddress[NI_MAXHOST];
>           /*
>            * TODO: we may have to make this bigger and interconnect
>            * specific for infinniband
>            */
>           char                    hwaddress[ISCSI_MAX_IFACE_LEN];
>           char
> transport_name[ISCSI_TRANSPORT_NAME_MAXLEN];
>           /*
>            * This is only used for boot now, but the iser guys
>            * can use this for their virtualization idea.
>            */
>           char                    alias[TARGET_NAME_MAXLEN + 1];
>           char                    iname[TARGET_NAME_MAXLEN + 1];
>           /*
>            * Following fields are used to support UIO driver based
>            * DHCP/ARP functionality for offload capable cards.
>            */
>           char                    nic_lib[PATH_MAX];
>           char                    nic_dev[PATH_MAX];
> } iface_rec_t;
> 
> B. Extend iface files to include following options
> "iface.nic_lib = vendor_lib.so"
> "iface.nic_dev = /dev/uioX"
> "iface.nic_args = additional SNIC specific args"
> 

Could we remove the option 'iface.nic_dev = /dev/uioX'?  The option
'iface.net_ifacename' should be enough to determine the uio char path.
'iface.net_ifacename' will tell iscsid what network interface to use.
Then iscsid could scan all the entries /sys/class/uio/uio*/device/ for
the file entry net:<net_ifacename>.  If there is a filename match then
iscsid has found the correct uio char path used to open the uio device.
This should simplify the configuration file or prevent conflicts with
the 'iface.net_ifacename' parameter.

> C. I have also modified set_net_config callback parameter in
> iscsi_transport_template as following, since we only need to have
> DHCP/STATIC and nic lib information while processing the nic config.
> 
> struct iscsi_transport_template {
>           const char *name;
>           uint8_t rdma;
>           int (*ep_connect) (struct iscsi_conn *conn, int non_blocking);
>           int (*ep_poll) (struct iscsi_conn *conn, int timeout_ms);
>           void (*ep_disconnect) (struct iscsi_conn *conn);
>           void (*create_conn) (struct iscsi_conn *conn);
>           int (*set_net_config) (struct iface_rec *iface);
> };

Agreed, we can remove the session parameter.

> 
> D. I have introduced new directory offload under open-iscsi for
> following purposes.
> 
> open-iscsi-head/offload
> open-iscsi-head/offload/vendor_lib => holds vendor specfic UIO library
> open-iscsi-head/offload/niclib => holds generic nic module
> open-iscsi-head/offload/uip => the uIP stack module
> open-iscsi-head/offload/uip/apps => holds various uip based module to
> provide dhcp and other functionality.
> 

Should these directories be placed under the 'usr' directory since they
will be linked with iscsid?

> E. nic_ops structure has been modified as following
> 
> typedef struct nic_ops {
>           struct nic_lib_ops      lib_ops;
> 
>           int (*open)(struct nic *);
>           int (*close)(struct nic *);
>           int (*read)(struct nic *, struct packet *);
>           int (*write)(struct nic *, nic_interface_t *,
>                        struct packet *);
>           int (*handle_irq) (struct nic *nic);
> } net_ops_t;
> 
> int (*handle_irq) (struct nic *nic)
> Instead of using global nic_process_intr we should use SNIC specific
> user space interrupt handling.
> 
> I have converted handle_iscsi_path_req callback into a direct call
> inside nic_nl.c, since its too generic.
> 
> Regards
> Rakesh Ranjan


Thanks again.

-Ben



--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To post to this group, send email to open-iscsi@googlegroups.com
To unsubscribe from this group, send email to 
open-iscsi+unsubscr...@googlegroups.com
For more options, visit this group at http://groups.google.com/group/open-iscsi
-~----------~----~----~----~------~----~------~--~---

Reply via email to