Re: [PATCH] staging: unisys: replace kzalloc/kfree with UISMALLOC/UISFREE

[Greg KH <gregkh@linuxfoundation.org>]

On Wed, Mar 19, 2014 at 04:58:52PM +0900, Daeseok Youn wrote:
> 
> It doesn't need to trace status of buffer when buffers are
> allocated/deallocated. So stuff of tracing memory status
> are removed. And also UISMALLOC/UISFREE macro are removed
> completetly. just use kzalloc/kfree.
> 
> Signed-off-by: Daeseok Youn <daeseok.youn@gmail.com>
> ---
>  drivers/staging/unisys/include/uisutils.h |    6 --
>  drivers/staging/unisys/uislib/uislib.c    |  109 +++-------------------------
>  drivers/staging/unisys/uislib/uisutils.c  |    5 +-
>  3 files changed, 14 insertions(+), 106 deletions(-)
> 
> diff --git a/drivers/staging/unisys/include/uisutils.h b/drivers/staging/unisys/include/uisutils.h
> index 81b5d9b..1c6d872 100644
> --- a/drivers/staging/unisys/include/uisutils.h
> +++ b/drivers/staging/unisys/include/uisutils.h
> @@ -340,12 +340,6 @@ static inline unsigned int Issue_VMCALL_FATAL_BYE_BYE(void)
>  }
>  
>  #define UIS_DAEMONIZE(nam)
> -void *uislib_malloc(size_t siz, gfp_t gfp, U8 contiguous, char *fn, int ln);
> -#define UISMALLOC(siz, gfp) uislib_malloc(siz, gfp, 1, __FILE__, __LINE__)
> -#define UISVMALLOC(siz) uislib_malloc(siz, 0, 0, __FILE__, __LINE__)
> -void uislib_free(void *p, size_t siz, U8 contiguous, char *fn, int ln);
> -#define UISFREE(p, siz) uislib_free(p, siz, 1, __FILE__, __LINE__)
> -#define UISVFREE(p, siz) uislib_free(p, siz, 0, __FILE__, __LINE__)
>  void *uislib_cache_alloc(struct kmem_cache *cur_pool, char *fn, int ln);
>  #define UISCACHEALLOC(cur_pool) uislib_cache_alloc(cur_pool, __FILE__, __LINE__)
>  void uislib_cache_free(struct kmem_cache *cur_pool, void *p, char *fn, int ln);
> diff --git a/drivers/staging/unisys/uislib/uislib.c b/drivers/staging/unisys/uislib/uislib.c
> index 1737f3a..c0eaa15 100644
> --- a/drivers/staging/unisys/uislib/uislib.c
> +++ b/drivers/staging/unisys/uislib/uislib.c
> @@ -316,7 +316,7 @@ create_bus(CONTROLVM_MESSAGE *msg, char *buf)
>  	size =
>  	    sizeof(struct bus_info) +
>  	    (deviceCount * sizeof(struct device_info *));
> -	bus = UISMALLOC(size, GFP_ATOMIC);
> +	bus = kzalloc(size, GFP_ATOMIC | __GFP_NORETRY);

All of the crazy __GFP_NORETRY stuff can be removed, no driver should
ever be using that.


>  	if (!bus) {
>  		LOGERR("CONTROLVM_BUS_CREATE Failed: kmalloc for bus failed.\n");
>  		POSTCODE_LINUX_3(BUS_CREATE_FAILURE_PC, busNo,
> @@ -324,8 +324,6 @@ create_bus(CONTROLVM_MESSAGE *msg, char *buf)
>  		return CONTROLVM_RESP_ERROR_KMALLOC_FAILED;
>  	}
>  
> -	memset(bus, 0, size);
> -
>  	/* Currently by default, the bus Number is the GuestHandle.
>  	 * Configure Bus message can override this.
>  	 */
> @@ -359,7 +357,7 @@ create_bus(CONTROLVM_MESSAGE *msg, char *buf)
>  		       bus->busNo);
>  		POSTCODE_LINUX_3(BUS_CREATE_FAILURE_PC, bus->busNo,
>  				 POSTCODE_SEVERITY_ERR);
> -		UISFREE(bus, size);
> +		kfree(bus);
>  		return CONTROLVM_RESP_ERROR_ALREADY_DONE;
>  	}
>  	if ((msg->cmd.createBus.channelAddr != 0)
> @@ -380,14 +378,14 @@ create_bus(CONTROLVM_MESSAGE *msg, char *buf)
>  		cmd.add_vbus.busTypeGuid = msg->cmd.createBus.busDataTypeGuid;
>  		cmd.add_vbus.busInstGuid = msg->cmd.createBus.busInstGuid;
>  		if (!VirtControlChanFunc) {
> -			UISFREE(bus, size);
> +			kfree(bus);
>  			LOGERR("CONTROLVM_BUS_CREATE Failed: virtpci callback not registered.");
>  			POSTCODE_LINUX_3(BUS_CREATE_FAILURE_PC, bus->busNo,
>  					 POSTCODE_SEVERITY_ERR);
>  			return CONTROLVM_RESP_ERROR_VIRTPCI_DRIVER_FAILURE;
>  		}
>  		if (!VirtControlChanFunc(&cmd)) {
> -			UISFREE(bus, size);
> +			kfree(bus);
>  			LOGERR("CONTROLVM_BUS_CREATE Failed: virtpci GUEST_ADD_VBUS returned error.");
>  			POSTCODE_LINUX_3(BUS_CREATE_FAILURE_PC, bus->busNo,
>  					 POSTCODE_SEVERITY_ERR);
> @@ -485,9 +483,7 @@ destroy_bus(CONTROLVM_MESSAGE *msg, char *buf)
>  		bus->pBusChannel = NULL;
>  	}
>  
> -	UISFREE(bus,
> -		sizeof(struct bus_info) +
> -		(bus->deviceCount * sizeof(struct device_info *)));
> +	kfree(bus);
>  	return CONTROLVM_RESP_SUCCESS;
>  }
>  
> @@ -507,7 +503,7 @@ create_device(CONTROLVM_MESSAGE *msg, char *buf)
>  	POSTCODE_LINUX_4(DEVICE_CREATE_ENTRY_PC, devNo, busNo,
>  			 POSTCODE_SEVERITY_INFO);
>  
> -	dev = UISMALLOC(sizeof(struct device_info), GFP_ATOMIC);
> +	dev = kzalloc(sizeof(struct device_info), GFP_ATOMIC | __GFP_NORETRY);
>  	if (!dev) {
>  		LOGERR("CONTROLVM_DEVICE_CREATE Failed: kmalloc for dev failed.\n");
>  		POSTCODE_LINUX_4(DEVICE_CREATE_FAILURE_PC, devNo, busNo,
> @@ -515,7 +511,6 @@ create_device(CONTROLVM_MESSAGE *msg, char *buf)
>  		return CONTROLVM_RESP_ERROR_KMALLOC_FAILED;
>  	}
>  
> -	memset(dev, 0, sizeof(struct device_info));
>  	dev->channelTypeGuid = msg->cmd.createDevice.dataTypeGuid;
>  	dev->intr = msg->cmd.createDevice.intr;
>  	dev->channelAddr = msg->cmd.createDevice.channelAddr;
> @@ -688,7 +683,7 @@ Away:
>  		dev->chanptr = NULL;
>  	}
>  
> -	UISFREE(dev, sizeof(struct device_info));
> +	kfree(dev);
>  	return result;
>  }
>  
> @@ -917,7 +912,7 @@ destroy_device(CONTROLVM_MESSAGE *msg, char *buf)
>  				LOGINF("destroy_device, doing iounmap");
>  				uislib_iounmap(dev->chanptr);
>  			}
> -			UISFREE(dev, sizeof(struct device_info));
> +			kfree(dev);
>  			bus->device[devNo] = NULL;
>  			break;
>  		}
> @@ -1343,69 +1338,7 @@ uislib_client_delete_vnic(U32 busNo)
>  	return 1;
>  }
>  EXPORT_SYMBOL_GPL(uislib_client_delete_vnic);
> -
> -				/* end client_delete_vnic */
> -
> -static atomic_t Malloc_BytesInUse = ATOMIC_INIT(0);
> -static atomic_t Malloc_BuffersInUse = ATOMIC_INIT(0);
> -static atomic_t Malloc_FailuresAlloc = ATOMIC_INIT(0);
> -static atomic_t Malloc_FailuresFree = ATOMIC_INIT(0);
> -static atomic_t Malloc_TotalMallocs = ATOMIC_INIT(0);
> -static atomic_t Malloc_TotalFrees = ATOMIC_INIT(0);
> -
> -void *
> -uislib_malloc(size_t siz, gfp_t gfp, U8 contiguous, char *fn, int ln)
> -{
> -	void *p = NULL;
> -
> -	if (contiguous == 0) {
> -		/* Allocate non-contiguous memory, such as in the
> -		* add_vnic and add_vhba methods where we are rebooting
> -		* the guest, for example.  Otherwise the contiguous
> -		* memory allocation attempt results in an
> -		* out-of-memory crash in the IOVM...
> -		*/
> -		p = vmalloc(siz);
> -	} else {
> -		/* __GFP_NORETRY means "ok to fail", meaning kmalloc()
> -		* can return NULL.  If you do NOT specify
> -		* __GFP_NORETRY, Linux will go to extreme measures to
> -		* get memory for you (like, invoke oom killer), which
> -		* will probably cripple the system.
> -		*/
> -		p = kmalloc(siz, gfp | __GFP_NORETRY);
> -	}
> -	if (p == NULL) {
> -		LOGERR("uislib_malloc failed to alloc %d bytes @%s:%d",
> -		       (int) siz, fn, ln);
> -		atomic_inc(&Malloc_FailuresAlloc);
> -		return NULL;
> -	}
> -	atomic_add((int) (siz), &Malloc_BytesInUse);
> -	atomic_inc(&Malloc_BuffersInUse);
> -	atomic_inc(&Malloc_TotalMallocs);	/* will eventually overflow */
> -	return p;
> -}
> -EXPORT_SYMBOL_GPL(uislib_malloc);
> -
> -void
> -uislib_free(void *p, size_t siz, U8 contiguous, char *fn, int ln)
> -{
> -	if (p == NULL) {
> -		LOGERR("uislib_free NULL pointer @%s:%d", fn, ln);
> -		atomic_inc(&Malloc_FailuresFree);
> -		return;
> -	}
> -
> -	if (contiguous == 0)
> -		vfree(p);
> -	else
> -		kfree(p);
> -	atomic_sub((int) (siz), &Malloc_BytesInUse);
> -	atomic_dec(&Malloc_BuffersInUse);
> -	atomic_inc(&Malloc_TotalFrees);	/* will eventually overflow */
> -}
> -EXPORT_SYMBOL_GPL(uislib_free);
> +/* end client_delete_vnic */
>  
>  void *
>  uislib_cache_alloc(struct kmem_cache *cur_pool, char *fn, int ln)
> @@ -1605,24 +1538,6 @@ info_proc_read_helper(char **buff, int *buff_len)
>  	}
>  	read_unlock(&BusListLock);
>  
> -	if (PLINE("Malloc bytes in use: %d\n",
> -		 atomic_read(&Malloc_BytesInUse)) < 0)
> -		goto err_done;
> -	if (PLINE("Malloc buffers in use: %d\n",
> -		  atomic_read(&Malloc_BuffersInUse)) < 0)
> -		goto err_done;
> -	if (PLINE("Malloc allocation failures: %d\n",
> -		  atomic_read(&Malloc_FailuresAlloc)) < 0)
> -		goto err_done;
> -	if (PLINE("Malloc free failures: %d\n",
> -		  atomic_read(&Malloc_FailuresFree)) < 0)
> -		goto err_done;
> -	if (PLINE("Malloc total mallocs: %u (may overflow)\n",
> -		  (unsigned) atomic_read(&Malloc_TotalMallocs)) < 0)
> -		goto err_done;
> -	if (PLINE("Malloc total frees: %u (may overflow)\n",
> -		  (unsigned) atomic_read(&Malloc_TotalFrees)) < 0)
> -		goto err_done;
>  	if (PLINE("UisUtils_Registered_Services: %d\n",
>  		  atomic_read(&UisUtils_Registered_Services)) < 0)
>  		goto err_done;
> @@ -1655,7 +1570,7 @@ info_proc_read(struct file *file, char __user *buf, size_t len, loff_t *offset)
>  /* *start = buf; */
>  	if (ProcReadBuffer == NULL) {
>  		DBGINF("ProcReadBuffer == NULL; allocating buffer.\n.");
> -		ProcReadBuffer = UISVMALLOC(PROC_READ_BUFFER_SIZE);
> +		ProcReadBuffer = vmalloc(PROC_READ_BUFFER_SIZE);
>  
>  		if (ProcReadBuffer == NULL) {
>  			LOGERR("failed to allocate buffer to provide proc data.\n");
> @@ -1691,7 +1606,7 @@ platformnumber_proc_read(struct file *file, char __user *buf,
>  	if (pos > 0 || !len)
>  		return 0;
>  
> -	vbuf = kzalloc(len, GFP_KERNEL);
> +	vbuf = kzalloc(len, GFP_KERNEL | __GFP_NORETRY);

Why did you change this line?

thanks,

greg k-h