[openib-general] RE: [PATCH][kdapl] Integrate dapl_hca_alloc/dapl_hca_free to dapl _provider.c
James Lentini
jlentini at netapp.com
Fri Jun 24 08:27:57 PDT 2005
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:jlentini at netapp.com]
>> 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 <itamar at mellanox.co.il>
>> 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>
>>
>
More information about the general
mailing list