[openib-general] RE: [PATCH][kdapl] Integrate dapl_hca_alloc/dapl_hca_free to dapl _provider.c

[James Lentini]

I think we are in violent agreement. :)
If we keep a dapl_hca structure, then it should be consistent managed 
in its own file for consistency. As we analyze how it is used, there 
doesn't seem to be a reason to keep it as an independent object.

I think grouping the information in dapl_hca, dapl_provider_entry, and 
the dat_provider into a new "dapl_provider" structure would be a good 
solution.

Keeping a list of the provider's IAs in this structure sounds like a 
good idea. uDAPL used the dapl_hca structure's ia_list to cleanup IA 
resources when the user space process forked. When the code was ported 
to the kernel, the list was retained, but the cleanup wasn't.

Suppose a kDAPL consumer (another kernel module) is unloaded and 
forgets to close an IA. What do we want to have happen when the kDAPL 
module is unloaded? Don't we want the resources associated with any 
open IA's cleaned up?

james

On Fri, 24 Jun 2005, Itamar Rabenstein wrote:

hi James,

I think that you are worng.
There is no functionalty to this struct.
it is only a struct that hold some data together.
if we look into the files dapl_hca_util.[h.c]
we will find 4 functions:
1) dapl_hca_alloc  ->  which is kmalloc and set to 4 data members of this
struct
2) dapl_hca_free   ->  only 1 line  == kfree
3) dapl_hca_link_ia    -> which is only a call to list_add
4) dapl_hca_unlink_ia  -> which is only a call to list_del

you have found out that functions 3 & 4 are not needed so we need to delete
them.

so i dont see why we should have 2 files for kmalloc and kfree

HCA is not like ia,evd,ep, ...
the others are dapl spec structs so they should be in separete files ,
hca is just a name that come from IB it is part of the provider code.
I believe that in other protocols implementaions they will not use "HCA".

Itamar

-----Original Message-----
From: James Lentini [mailto:[EMAIL PROTECTED]
Sent: Thursday, June 23, 2005 11:27 PM
To: Itamar Rabenstein
Cc: openib-general
Subject: Re: [PATCH][kdapl] Integrate dapl_hca_alloc/dapl_hca_free to
dapl_provider.c



For consistency, I think we should keep the HCA object in its own
file.

However, I'm not sure we need an HCA object. Is there a better way to
organize the data being stored in the dapl_hca structure? I have this
feeling that we should merge all the structures that we create on a
per-hca basis (the dapl_provider_entry, dat_provider table, and
dapl_hca).

james

On Mon, 20 Jun 2005, Itamar Rabenstein wrote:

itamar> ntegrate dapl_hca_alloc/dapl_hca_free to dapl_provider.c
itamar> (no need for 2 files just for 2 simple function that
kmalloc and kfree.
itamar>  There is not any special logic in this functions
that need to separate
itamar>  them into different files)
itamar>
itamar> Signed-off-by: Itamar Rabenstein <[EMAIL PROTECTED]>
itamar>
itamar> diff -Nurp -X dontdiff dat-provider_link_ia/Makefile
dat-provider/Makefile
itamar> --- dat-provider_link_ia/Makefile    Sun Jun 19 18:43:16 2005
itamar> +++ dat-provider/Makefile    Sun Jun 19 19:45:15 2005
itamar> @@ -20,7 +20,6 @@ PROVIDER_MODULES := \
itamar>          dapl_cr                     \
itamar>          dapl_ep                         \
itamar>          dapl_evd                        \
itamar> -        dapl_hca_util                       \
itamar>          dapl_ia                     \
itamar>          dapl_lmr                    \
itamar>          dapl_provider                       \
itamar> diff -Nurp -X dontdiff
dat-provider_link_ia/dapl_hca_util.c dat-provider/dapl_hca_util.c
itamar> --- dat-provider_link_ia/dapl_hca_util.c     Sun Jun
19 18:43:16 2005
itamar> +++ dat-provider/dapl_hca_util.c     Thu Jan  1 02:00:00 1970
itamar> @@ -1,90 +0,0 @@
itamar> -/*
itamar> - * Copyright (c) 2002-2005, Network Appliance, Inc.
All rights reserved.
itamar> - * Copyright (c) 2005 Sun Microsystems, Inc. All
rights reserved.
itamar> - *
itamar> - * This Software is licensed under one of the
following licenses:
itamar> - *
itamar> - * 1) under the terms of the "Common Public License
1.0" a copy of which is
itamar> - *    available from the Open Source Initiative, see
itamar> - *    http://www.opensource.org/licenses/cpl.php.
itamar> - *
itamar> - * 2) under the terms of the "The BSD License" a
copy of which is
itamar> - *    available from the Open Source Initiative, see
itamar> - *    http://www.opensource.org/licenses/bsd-license.php.
itamar> - *
itamar> - * 3) under the terms of the "GNU General Public
License (GPL) Version 2" a
itamar> - *    copy of which is available from the Open
Source Initiative, see
itamar> - *    http://www.opensource.org/licenses/gpl-license.php.
itamar> - *
itamar> - * Licensee has the right to choose one of the above
licenses.
itamar> - *
itamar> - * Redistributions of source code must retain the
above copyright
itamar> - * notice and one of the license notices.
itamar> - *
itamar> - * Redistributions in binary form must reproduce
both the above copyright
itamar> - * notice, one of the license notices in the documentation
itamar> - * and/or other materials provided with the distribution.
itamar> - */
itamar> -
itamar> -/*
itamar> - * $Id: dapl_hca_util.c 2640 2005-06-16 16:22:46Z jlentini $
itamar> - */
itamar> -
itamar> -#include "dapl.h"
itamar> -#include "dapl_openib_util.h"
itamar> -#include "dapl_provider.h"
itamar> -#include "dapl_hca_util.h"
itamar> -
itamar> -/*
itamar> - * dapl_hca_alloc
itamar> - *
itamar> - * alloc and initialize an HCA struct
itamar> - *
itamar> - * Input:
itamar> - *  name
itamar> - *      port
itamar> - *
itamar> - * Output:
itamar> - *  hca
itamar> - *
itamar> - * Returns:
itamar> - *  none
itamar> - *
itamar> - */
itamar> -struct dapl_hca *dapl_hca_alloc(char *name, struct
ib_device *device, u8 port)
itamar> -{
itamar> -    struct dapl_hca *hca;
itamar> -    int malloc_size = sizeof *hca + strlen(name) + 1;
itamar> -
itamar> -    hca = kmalloc(malloc_size, GFP_ATOMIC);
itamar> -    if (hca) {
itamar> -            memset(hca, 0, malloc_size);
itamar> -            spin_lock_init(&hca->lock);
itamar> -            INIT_LIST_HEAD(&hca->ia_list);
itamar> -            hca->name = (char *)hca + sizeof *hca;
itamar> -            strcpy(hca->name, name);
itamar> -            hca->ib_hca_handle = device;
itamar> -            hca->port_num = port;
itamar> -    }
itamar> -    return hca;
itamar> -}
itamar> -
itamar> -/*
itamar> - * dapl_hca_free
itamar> - *
itamar> - * free an IA INFO struct
itamar> - *
itamar> - * Input:
itamar> - *  hca
itamar> - *
itamar> - * Output:
itamar> - *  none
itamar> - *
itamar> - * Returns:
itamar> - *  none
itamar> - *
itamar> - */
itamar> -void dapl_hca_free(struct dapl_hca *hca)
itamar> -{
itamar> -    kfree(hca);
itamar> -}
itamar> diff -Nurp -X dontdiff
dat-provider_link_ia/dapl_hca_util.h dat-provider/dapl_hca_util.h
itamar> --- dat-provider_link_ia/dapl_hca_util.h     Sun Jun
19 18:43:16 2005
itamar> +++ dat-provider/dapl_hca_util.h     Thu Jan  1 02:00:00 1970
itamar> @@ -1,42 +0,0 @@
itamar> -/*
itamar> - * Copyright (c) 2002-2005, Network Appliance, Inc.
All rights reserved.
itamar> - * Copyright (c) 2005 Sun Microsystems, Inc. All
rights reserved.
itamar> - *
itamar> - * This Software is licensed under one of the
following licenses:
itamar> - *
itamar> - * 1) under the terms of the "Common Public License
1.0" a copy of which is
itamar> - *    available from the Open Source Initiative, see
itamar> - *    http://www.opensource.org/licenses/cpl.php.
itamar> - *
itamar> - * 2) under the terms of the "The BSD License" a
copy of which is
itamar> - *    available from the Open Source Initiative, see
itamar> - *    http://www.opensource.org/licenses/bsd-license.php.
itamar> - *
itamar> - * 3) under the terms of the "GNU General Public
License (GPL) Version 2" a
itamar> - *    copy of which is available from the Open
Source Initiative, see
itamar> - *    http://www.opensource.org/licenses/gpl-license.php.
itamar> - *
itamar> - * Licensee has the right to choose one of the above
licenses.
itamar> - *
itamar> - * Redistributions of source code must retain the
above copyright
itamar> - * notice and one of the license notices.
itamar> - *
itamar> - * Redistributions in binary form must reproduce
both the above copyright
itamar> - * notice, one of the license notices in the documentation
itamar> - * and/or other materials provided with the distribution.
itamar> - */
itamar> -
itamar> -/*
itamar> - * $Id: dapl_hca_util.h 2640 2005-06-16 16:22:46Z jlentini $
itamar> - */
itamar> -
itamar> -#ifndef DAPL_HCA_UTIL_H
itamar> -#define DAPL_HCA_UTIL_H
itamar> -
itamar> -#include "dapl.h"
itamar> -
itamar> -struct dapl_hca *dapl_hca_alloc(char *name, struct
ib_device *device, u8 port);
itamar> -
itamar> -void dapl_hca_free(struct dapl_hca *hca);
itamar> -
itamar> -#endif
itamar> diff -Nurp -X dontdiff
dat-provider_link_ia/dapl_provider.c dat-provider/dapl_provider.c
itamar> --- dat-provider_link_ia/dapl_provider.c     Sun Jun
19 18:43:16 2005
itamar> +++ dat-provider/dapl_provider.c     Sun Jun 19 19:47:16 2005
itamar> @@ -35,7 +35,6 @@
itamar>  #include <dat.h>
itamar>
itamar>  #include "dapl.h"
itamar> -#include "dapl_hca_util.h"
itamar>  #include "dapl_provider.h"
itamar>  #include "dapl_util.h"
itamar>  #include "dapl_openib_util.h"
itamar> @@ -246,6 +245,24 @@ static void dapl_provider_info_init(stru
itamar>      provider_info->ia_name[i+1] = '\0';
itamar>  }
itamar>
itamar> +static struct dapl_hca *dapl_hca_alloc(char *name,
struct ib_device *device, u8 port)
itamar> +{
itamar> +    struct dapl_hca *hca;
itamar> +    int malloc_size = sizeof *hca + strlen(name) + 1;
itamar> +
itamar> +    hca = kmalloc(malloc_size, GFP_ATOMIC);
itamar> +    if (hca) {
itamar> +            memset(hca, 0, malloc_size);
itamar> +            spin_lock_init(&hca->lock);
itamar> +            INIT_LIST_HEAD(&hca->ia_list);
itamar> +            hca->name = (char *)hca + sizeof *hca;
itamar> +            strcpy(hca->name, name);
itamar> +            hca->ib_hca_handle = device;
itamar> +            hca->port_num = port;
itamar> +    }
itamar> +    return hca;
itamar> +}
itamar> +
itamar>  static void dapl_add_port(struct ib_device *device, u8 port)
itamar>  {
itamar>      struct dat_provider_info provider_info;
itamar> @@ -305,7 +322,7 @@ error:
itamar>
(void)dapl_provider_list_remove(provider_info.ia_name);
itamar>
itamar>              if (NULL != hca)
itamar> -                    dapl_hca_free(hca);
itamar> +                    kfree(hca);
itamar>      }
itamar>  }
itamar>
itamar> @@ -338,7 +355,7 @@ static void dapl_remove_port(struct ib_d
itamar>                           provider_info.ia_name);
itamar>      }
itamar>
itamar> -    dapl_hca_free(provider->extension);
itamar> +    kfree(provider->extension);
itamar>
itamar>      dapl_provider_list_remove(provider_info.ia_name);
itamar>  }
itamar> --
itamar> Itamar
itamar>


_______________________________________________
openib-general mailing list
openib-general@openib.org
http://openib.org/mailman/listinfo/openib-general

To unsubscribe, please visit http://openib.org/mailman/listinfo/openib-general