Re: [PATCH 3/6] GenWQE Utility functions

[Frank Haverkamp]

Hi Greg,

Am Mittwoch, den 27.11.2013, 11:26 -0800 schrieb Greg KH:
> On Wed, Nov 06, 2013 at 01:45:40PM +0100, Frank Haverkamp wrote:
> > +/**
> > + * init_crc32() - Prepare a lookup table for fast crc32 calculations
> > + *
> > + * Existing kernel functions seem to use a different polynom,
> > + * therefore we could not use them here.
> > + *
> > + * Genwqe's Polynomial = 0x20044009
> > + */
> > +#define CRC32_POLYNOMIAL	0x20044009
> > +static u32 crc32_tab[256];	/* crc32 lookup table */
> > +
> > +void init_crc32(void)
> 
> That's a _very_ generic function name to now be global in the whole
> kernel.
> 
> And why not just add new polynomial functionality to the existing crc32
> in-kernel code instead of rolling your own here?
> 

The name is indeed to generic. I fixed that. It is for sure a candidate
to be covered by the in-kernel crc support. I had a look and found that
 
   static void crc32init_be(void)

is unfortunately defined static and that the polynomial is missing.
Maybe:

   void crc32init_be_generic(const uint32_t polynomial,
                             uint32_t (*tab)[256])

   inline u32 __pure crc32_be_generic(u32 crc, unsigned char const *p,
                                     size_t len, const u32 (*tab)[256],
                                     u32 polynomial)

would be good to have. Adding my own "crc32table_genwqe_be[]" to the
generic code might be waste memory. None except of us are using that
particular polynomial, as far as I saw. So enhancing the table
generation function and exporting that and the crc32_be_generic might be
a good idea.

In my latest version of the patchset I did not include such a proposal
(yet).

> > +static void genwqe_dump_sgl(struct genwqe_dev *cd, struct sg_entry *sgl,
> > +		    size_t sgl_size)
> > +{
> > +	unsigned int i, j;
> > +	struct pci_dev *pci_dev = cd->pci_dev;
> > +
> > +	for (j = 0, i = 0; i < sgl_size/sizeof(struct sg_entry); i++, j++) {
> > +		if (j == 8) {
> > +			dev_info(&pci_dev->dev, "  --\n");
> > +			j = 0;
> > +		}
> > +		dev_info(&pci_dev->dev, "  %016llx %08x %08x %s\n",
> > +			 be64_to_cpu(sgl[i].target_addr),
> > +			 be32_to_cpu(sgl[i].len),
> > +			 be32_to_cpu(sgl[i].flags),
> > +			 (be32_to_cpu(sgl[i].len) > PAGE_SIZE) ? "C" : "");
> > +
> > +		if (be32_to_cpu(sgl[i].flags) == SG_END_LIST)
> > +			break;
> > +	}
> > +}
> 
> That's a lot of kernel log mess, why?

I removed it. It was for debugging the scatter gather lists our hardware
uses. Since it works now, I can get rid of the printing.

> > +/**
> > + * free_user_pages() - Give pinned pages back
> > + *
> > + * Documentation of get_user_pages is in mm/memory.c:
> > + *
> > + * If the page is written to, set_page_dirty (or set_page_dirty_lock,
> > + * as appropriate) must be called after the page is finished with, and
> > + * before put_page is called.
> > + */
> > +static int free_user_pages(struct page **page_list, unsigned int nr_pages,
> > +			   int dirty)
> > +{
> > +	unsigned int i;
> > +
> > +	for (i = 0; i < nr_pages; i++) {
> > +		if (page_list[i] != NULL) {
> > +			if (dirty)
> > +				set_page_dirty_lock(page_list[i]);
> > +			put_page(page_list[i]);
> > +		}
> > +	}
> > +	return 0;
> > +}
> > +
> > +/**
> > + * user_vmap() - Map user-space memory to virtual kernel memory
> > + * @cd:         pointer to genwqe device
> > + * @m:          mapping params
> > + * @uaddr:      user virtual address
> > + * @size:       size of memory to be mapped
> > + *
> > + * We need to think about how we could speed this up. Of course it is
> > + * not a good idea to do this over and over again, like we are
> > + * currently doing it. Nevertheless, I am curious where on the path
> > + * the performance is spend. Most probably within the memory
> > + * allocation functions, but maybe also in the DMA mapping code.
> > + *
> > + * Restrictions: The maximum size of the possible mapping currently depends
> > + *               on the amount of memory we can get using kzalloc() for the
> > + *               page_list and pci_alloc_coherent for the sg_list.
> > + *               The sg_list is currently itself not scattered, which could
> > + *               be fixed with some effort. The page_list must be split into
> > + *               PAGE_SIZE chunks too. All that will make the complicated
> > + *               code more complicated.
> > + *
> > + * Return: 0 if success
> > + */
> > +int user_vmap(struct genwqe_dev *cd, struct dma_mapping *m, void *uaddr,
> > +	      unsigned long size, struct ddcb_requ *req)
> 
> Again, _very_ generic global symbol name.  Please audit all of these.

I prefixed those and found some others too. I hope that I have found all
similar cases in our code.

> And shouldn't this just be a core mm function?  Why is this in a driver?

The current implementation is at the moment not generic enough to be
useful for others. But you are right, something alike might be useful if
there is a 2nd or 3rd user with same/similar needs.

> > +/**
> > + * user_vunmap() - Undo mapping of user-space memory to virtual kernel memory
> > + * @cd:         pointer to genwqe device
> > + * @m:          mapping params
> > + */
> > +int user_vunmap(struct genwqe_dev *cd, struct dma_mapping *m,
> > +		struct ddcb_requ *req)
> 
> Same as above.
> 
> thanks,
> 
> greg k-h
> 

Regards

Frank

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/