Re: [PATCH v2 07/21] crypto: shash, caam: Make use of the new sg_map helper function
On Thu, Apr 27, 2017 at 09:45:57AM -0600, Logan Gunthorpe wrote: > > > On 26/04/17 09:56 PM, Herbert Xu wrote: > > On Tue, Apr 25, 2017 at 12:20:54PM -0600, Logan Gunthorpe wrote: > >> Very straightforward conversion to the new function in the caam driver > >> and shash library. > >> > >> Signed-off-by: Logan Gunthorpe > >> Cc: Herbert Xu > >> Cc: "David S. Miller" > >> --- > >> crypto/shash.c| 9 ++--- > >> drivers/crypto/caam/caamalg.c | 8 +++- > >> 2 files changed, 9 insertions(+), 8 deletions(-) > >> > >> diff --git a/crypto/shash.c b/crypto/shash.c > >> index 5e31c8d..5914881 100644 > >> --- a/crypto/shash.c > >> +++ b/crypto/shash.c > >> @@ -283,10 +283,13 @@ int shash_ahash_digest(struct ahash_request *req, > >> struct shash_desc *desc) > >>if (nbytes < min(sg->length, ((unsigned int)(PAGE_SIZE)) - offset)) { > >>void *data; > >> > >> - data = kmap_atomic(sg_page(sg)); > >> - err = crypto_shash_digest(desc, data + offset, nbytes, > >> + data = sg_map(sg, 0, SG_KMAP_ATOMIC); > >> + if (IS_ERR(data)) > >> + return PTR_ERR(data); > >> + > >> + err = crypto_shash_digest(desc, data, nbytes, > >> req->result); > >> - kunmap_atomic(data); > >> + sg_unmap(sg, data, 0, SG_KMAP_ATOMIC); > >>crypto_yield(desc->flags); > >>} else > >>err = crypto_shash_init(desc) ?: > > > > Nack. This is an optimisation for the special case of a single > > SG list entry. In fact in the common case the kmap_atomic should > > disappear altogether in the no-highmem case. So replacing it > > with sg_map is not acceptable. > > What you seem to have missed is that sg_map is just a thin wrapper > around kmap_atomic. Perhaps with a future check for a mappable page. > This change should have zero impact on performance. You are right. Indeed the existing code looks buggy as they don't take sg->offset into account when doing the kmap. Could you send me some patches that fix these problems first so that they can be easily backported? Thanks, -- Email: Herbert Xu Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt -- You received this message because you are subscribed to the Google Groups "open-iscsi" group. To unsubscribe from this group and stop receiving emails from it, send an email to open-iscsi+unsubscr...@googlegroups.com. To post to this group, send email to open-iscsi@googlegroups.com. Visit this group at https://groups.google.com/group/open-iscsi. For more options, visit https://groups.google.com/d/optout.
Re: [PATCH] iscsid: Add qedi ping transport hook
Applied. Please note that pull requests via github are easier to apply than traditional patch emails. On Wednesday, April 26, 2017 at 2:27:03 AM UTC-7, Nilesh Javali wrote: > > iscsiuio ping is operational for qedi. > Add missing qedi transport hook for ping support. > > Signed-off-by: Nilesh Javali > --- > usr/transport.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/usr/transport.c b/usr/transport.c > index 533ba30..56fab49 100644 > --- a/usr/transport.c > +++ b/usr/transport.c > @@ -124,6 +124,7 @@ struct iscsi_transport_template qedi = { > .ep_poll= ktransport_ep_poll, > .ep_disconnect= ktransport_ep_disconnect, > .set_net_config = uip_broadcast_params, > +.exec_ping = uip_broadcast_ping_req, > }; > > static struct iscsi_transport_template *iscsi_transport_templates[] = { > -- > 1.8.3.1 > > -- You received this message because you are subscribed to the Google Groups "open-iscsi" group. To unsubscribe from this group and stop receiving emails from it, send an email to open-iscsi+unsubscr...@googlegroups.com. To post to this group, send email to open-iscsi@googlegroups.com. Visit this group at https://groups.google.com/group/open-iscsi. For more options, visit https://groups.google.com/d/optout.
Re: [PATCH v2 15/21] xen-blkfront: Make use of the new sg_map helper function
On 27/04/17 05:20 PM, Jason Gunthorpe wrote: > It seems the most robust: test for iomem, and jump to a slow path > copy, otherwise inline the kmap and memcpy > > Every place doing memcpy from sgl will need that pattern to be > correct. Ok, sounds like a good place to start to me. I'll see what I can do for a v3 of this set. Though, I probably won't send anything until after the merge window. >>> sg_miter will still fail when the sg contains __iomem, however I would >>> expect that the sg_copy will work with iomem, by using the __iomem >>> memcpy variant. >> >> Yes, that's true. Any sg_miters that ever see iomem will need to be >> converted to support it. This isn't much different than the other >> kmap(sg_page()) users I was converting that will also fail if they see >> iomem. Though, I suspect an sg_miter user would be easier to convert to >> iomem than a random kmap user. > > How? sg_miter seems like the next nightmare down this path, what is > sg_miter_next supposed to do when something hits an iomem sgl? My proposal is roughly included in the draft I sent upthread. We add an sg_miter flag indicating the iteratee supports iomem and if miter finds iomem (with the support flag set) it sets ioaddr which is __iomem. The iteratee then just needs to null check addr and ioaddr and perform the appropriate action. Logan -- You received this message because you are subscribed to the Google Groups "open-iscsi" group. To unsubscribe from this group and stop receiving emails from it, send an email to open-iscsi+unsubscr...@googlegroups.com. To post to this group, send email to open-iscsi@googlegroups.com. Visit this group at https://groups.google.com/group/open-iscsi. For more options, visit https://groups.google.com/d/optout.
Re: [PATCH v2 15/21] xen-blkfront: Make use of the new sg_map helper function
On 27/04/17 04:11 PM, Jason Gunthorpe wrote: > On Thu, Apr 27, 2017 at 03:53:37PM -0600, Logan Gunthorpe wrote: > Well, that is in the current form, with more users it would make sense > to optimize for the single page case, eg by providing the existing > call, providing a faster single-page-only variant of the copy, perhaps > even one that is inlined. Ok, does it make sense then to have an sg_copy_page_to_buffer (or some such... I'm having trouble thinking of a sane name that isn't too long). That just does k(un)map_atomic and memcpy? I could try that if it makes sense to people. >> Switching the for_each_sg to sg_miter is probably the nicer solution as >> it takes care of the mapping and the offset/length accounting for you >> and will have similar performance. > > sg_miter will still fail when the sg contains __iomem, however I would > expect that the sg_copy will work with iomem, by using the __iomem > memcpy variant. Yes, that's true. Any sg_miters that ever see iomem will need to be converted to support it. This isn't much different than the other kmap(sg_page()) users I was converting that will also fail if they see iomem. Though, I suspect an sg_miter user would be easier to convert to iomem than a random kmap user. Logan -- You received this message because you are subscribed to the Google Groups "open-iscsi" group. To unsubscribe from this group and stop receiving emails from it, send an email to open-iscsi+unsubscr...@googlegroups.com. To post to this group, send email to open-iscsi@googlegroups.com. Visit this group at https://groups.google.com/group/open-iscsi. For more options, visit https://groups.google.com/d/optout.
Re: [PATCH v2 15/21] xen-blkfront: Make use of the new sg_map helper function
On 27/04/17 02:53 PM, Jason Gunthorpe wrote: > blkfront is one of the drivers I looked at, and it appears to only be > memcpying with the bvec_data pointer, so I wonder why it does not use > sg_copy_X_buffer instead.. Yes, sort of... But you'd potentially end up calling sg_copy_to_buffer multiple times per page within the sg (given that gnttab_foreach_grant_in_range might call blkif_copy_from_grant/blkif_setup_rw_req_grant multiple times). Even calling sg_copy_to_buffer once per page seems rather inefficient as it uses sg_miter internally. Switching the for_each_sg to sg_miter is probably the nicer solution as it takes care of the mapping and the offset/length accounting for you and will have similar performance. But, yes, if performance is not an issue, switching it to sg_copy_to_buffer would be a less invasive change than sg_miter. Which the same might be said about a lot of these cases. Unfortunately, changing from kmap_atomic (which is a null operation in a lot of cases) to sg_copy_X_buffer is a pretty big performance hit. Logan -- You received this message because you are subscribed to the Google Groups "open-iscsi" group. To unsubscribe from this group and stop receiving emails from it, send an email to open-iscsi+unsubscr...@googlegroups.com. To post to this group, send email to open-iscsi@googlegroups.com. Visit this group at https://groups.google.com/group/open-iscsi. For more options, visit https://groups.google.com/d/optout.
Re: [PATCH v2 15/21] xen-blkfront: Make use of the new sg_map helper function
On 26/04/17 01:37 AM, Roger Pau Monné wrote: > On Tue, Apr 25, 2017 at 12:21:02PM -0600, Logan Gunthorpe wrote: >> Straightforward conversion to the new helper, except due to the lack >> of error path, we have to use SG_MAP_MUST_NOT_FAIL which may BUG_ON in >> certain cases in the future. >> >> Signed-off-by: Logan Gunthorpe >> Cc: Boris Ostrovsky >> Cc: Juergen Gross >> Cc: Konrad Rzeszutek Wilk >> Cc: "Roger Pau Monné" >> --- >> drivers/block/xen-blkfront.c | 20 +++- >> 1 file changed, 11 insertions(+), 9 deletions(-) >> >> diff --git a/drivers/block/xen-blkfront.c b/drivers/block/xen-blkfront.c >> index 3945963..ed62175 100644 >> --- a/drivers/block/xen-blkfront.c >> +++ b/drivers/block/xen-blkfront.c >> @@ -816,8 +816,9 @@ static int blkif_queue_rw_req(struct request *req, >> struct blkfront_ring_info *ri >> BUG_ON(sg->offset + sg->length > PAGE_SIZE); >> >> if (setup.need_copy) { >> -setup.bvec_off = sg->offset; >> -setup.bvec_data = kmap_atomic(sg_page(sg)); >> +setup.bvec_off = 0; >> +setup.bvec_data = sg_map(sg, 0, SG_KMAP_ATOMIC | >> + SG_MAP_MUST_NOT_FAIL); > > I assume that sg_map already adds sg->offset to the address? Correct. > Also wondering whether we can get rid of bvec_off and just increment > bvec_data, > adding Julien who IIRC added this code. bvec_off is used to keep track of the offset within the current mapping so it's not a great idea given that you'd want to kunmap_atomic the original address and not something with an offset. It would be nice if this could be converted to use the sg_miter interface but that's a much more invasive change that would require someone who knows this code and can properly test it. I'd be very grateful if someone actually took that on. Logan -- You received this message because you are subscribed to the Google Groups "open-iscsi" group. To unsubscribe from this group and stop receiving emails from it, send an email to open-iscsi+unsubscr...@googlegroups.com. To post to this group, send email to open-iscsi@googlegroups.com. Visit this group at https://groups.google.com/group/open-iscsi. For more options, visit https://groups.google.com/d/optout.
Re: [PATCH v2 01/21] scatterlist: Introduce sg_map helper functions
On 26/04/17 01:44 AM, Christoph Hellwig wrote: > I think we'll at least need a draft of those to make sense of these > patches. Otherwise they just look very clumsy. Ok, what follows is a draft patch attempting to show where I'm thinking of going with this. Obviously it will not compile because it assumes the users throughout the kernel are a bit different than they are today. Notably, there is no sg_page anymore. There's also likely a ton of issues and arguments to have over a bunch of the specifics below and I'd expect the concept to evolve more as cleanup occurs. This itself is an evolution of the draft I posted replying to you in my last RFC thread. Also, before any of this is truly useful to us, pfn_t would have to infect a few other places in the kernel. Thanks, Logan diff --git a/include/linux/scatterlist.h b/include/linux/scatterlist.h index fad170b..85ef928 100644 --- a/include/linux/scatterlist.h +++ b/include/linux/scatterlist.h @@ -6,13 +6,14 @@ #include #include #include +#include #include struct scatterlist { #ifdef CONFIG_DEBUG_SG unsigned long sg_magic; #endif - unsigned long page_link; + pfn_t pfn; unsigned intoffset; unsigned intlength; dma_addr_t dma_address; @@ -60,15 +61,68 @@ struct sg_table { #define SG_MAGIC 0x87654321 -/* - * We overload the LSB of the page pointer to indicate whether it's - * a valid sg entry, or whether it points to the start of a new scatterlist. - * Those low bits are there for everyone! (thanks mason :-) - */ -#define sg_is_chain(sg)((sg)->page_link & 0x01) -#define sg_is_last(sg) ((sg)->page_link & 0x02) -#define sg_chain_ptr(sg) \ - ((struct scatterlist *) ((sg)->page_link & ~0x03)) +static inline bool sg_is_chain(struct scatterlist *sg) +{ + return sg->pfn.val & PFN_SG_CHAIN; +} + +static inline bool sg_is_last(struct scatterlist *sg) +{ + return sg->pfn.val & PFN_SG_LAST; +} + +static inline struct scatterlist *sg_chain_ptr(struct scatterlist *sg) +{ + unsigned long sgl = pfn_t_to_pfn(sg->pfn); + return (struct scatterlist *)(sgl << PAGE_SHIFT); +} + +static inline bool sg_is_iomem(struct scatterlist *sg) +{ + return pfn_t_is_iomem(sg->pfn); +} + +/** + * sg_assign_pfn - Assign a given pfn_t to an SG entry + * @sg:SG entry + * @pfn: The pfn + * + * Description: + * Assign a pfn to sg entry. Also see sg_set_pfn(), the most commonly used + * variant.w + * + **/ +static inline void sg_assign_pfn(struct scatterlist *sg, pfn_t pfn) +{ +#ifdef CONFIG_DEBUG_SG + BUG_ON(sg->sg_magic != SG_MAGIC); + BUG_ON(sg_is_chain(sg)); + BUG_ON(pfn.val & (PFN_SG_CHAIN | PFN_SG_LAST)); +#endif + + sg->pfn = pfn; +} + +/** + * sg_set_pfn - Set sg entry to point at given pfn + * @sg: SG entry + * @pfn:The page + * @len:Length of data + * @offset: Offset into page + * + * Description: + * Use this function to set an sg entry pointing at a pfn, never assign + * the page directly. We encode sg table information in the lower bits + * of the page pointer. See sg_pfn_t for looking up the pfn_t belonging + * to an sg entry. + **/ +static inline void sg_set_pfn(struct scatterlist *sg, pfn_t pfn, + unsigned int len, unsigned int offset) +{ + sg_assign_pfn(sg, pfn); + sg->offset = offset; + sg->length = len; +} /** * sg_assign_page - Assign a given page to an SG entry @@ -82,18 +136,13 @@ struct sg_table { **/ static inline void sg_assign_page(struct scatterlist *sg, struct page *page) { - unsigned long page_link = sg->page_link & 0x3; + if (!page) { + pfn_t null_pfn = {0}; + sg_assign_pfn(sg, null_pfn); + return; + } - /* -* In order for the low bit stealing approach to work, pages -* must be aligned at a 32-bit boundary as a minimum. -*/ - BUG_ON((unsigned long) page & 0x03); -#ifdef CONFIG_DEBUG_SG - BUG_ON(sg->sg_magic != SG_MAGIC); - BUG_ON(sg_is_chain(sg)); -#endif - sg->page_link = page_link | (unsigned long) page; + sg_assign_pfn(sg, page_to_pfn_t(page)); } /** @@ -106,8 +155,7 @@ static inline void sg_assign_page(struct scatterlist *sg, struct page *page) * Description: * Use this function to set an sg entry pointing at a page, never assign * the page directly. We encode sg table information in the lower bits - * of the page pointer. See sg_page() for looking up the page belonging - * to an sg entry. + * of the page pointer. * **/ static inline void sg_set_page(struct scatterlist *sg, struct page *page, @@ -118,13 +166,53 @@ static inline void sg_set_page(struct scatterlist *sg, struct page *page, sg->length = len; } -static inline struct page *sg_page(struct scatterlist *sg) +/** + * sg_pfn_t - Return the pfn_
Re: [PATCH v2 01/21] scatterlist: Introduce sg_map helper functions
On 27/04/17 09:27 AM, Jason Gunthorpe wrote: > On Thu, Apr 27, 2017 at 08:53:38AM +0200, Christoph Hellwig wrote: > How about first switching as many call sites as possible to use > sg_copy_X_buffer instead of kmap? Yeah, I could look at doing that first. One problem is we might get more Naks of the form of Herbert Xu's who might be concerned with the performance implications. These are definitely a bit more invasive changes than thin wrappers around kmap calls. > A random audit of Logan's series suggests this is actually a fairly > common thing. It's not _that_ common but there are a significant fraction. One of my patches actually did this to two places that seemed to be reimplementing the sg_copy_X_buffer logic. Thanks, Logan -- You received this message because you are subscribed to the Google Groups "open-iscsi" group. To unsubscribe from this group and stop receiving emails from it, send an email to open-iscsi+unsubscr...@googlegroups.com. To post to this group, send email to open-iscsi@googlegroups.com. Visit this group at https://groups.google.com/group/open-iscsi. For more options, visit https://groups.google.com/d/optout.
Re: [PATCH v2 07/21] crypto: shash, caam: Make use of the new sg_map helper function
On 26/04/17 09:56 PM, Herbert Xu wrote: > On Tue, Apr 25, 2017 at 12:20:54PM -0600, Logan Gunthorpe wrote: >> Very straightforward conversion to the new function in the caam driver >> and shash library. >> >> Signed-off-by: Logan Gunthorpe >> Cc: Herbert Xu >> Cc: "David S. Miller" >> --- >> crypto/shash.c| 9 ++--- >> drivers/crypto/caam/caamalg.c | 8 +++- >> 2 files changed, 9 insertions(+), 8 deletions(-) >> >> diff --git a/crypto/shash.c b/crypto/shash.c >> index 5e31c8d..5914881 100644 >> --- a/crypto/shash.c >> +++ b/crypto/shash.c >> @@ -283,10 +283,13 @@ int shash_ahash_digest(struct ahash_request *req, >> struct shash_desc *desc) >> if (nbytes < min(sg->length, ((unsigned int)(PAGE_SIZE)) - offset)) { >> void *data; >> >> -data = kmap_atomic(sg_page(sg)); >> -err = crypto_shash_digest(desc, data + offset, nbytes, >> +data = sg_map(sg, 0, SG_KMAP_ATOMIC); >> +if (IS_ERR(data)) >> +return PTR_ERR(data); >> + >> +err = crypto_shash_digest(desc, data, nbytes, >>req->result); >> -kunmap_atomic(data); >> +sg_unmap(sg, data, 0, SG_KMAP_ATOMIC); >> crypto_yield(desc->flags); >> } else >> err = crypto_shash_init(desc) ?: > > Nack. This is an optimisation for the special case of a single > SG list entry. In fact in the common case the kmap_atomic should > disappear altogether in the no-highmem case. So replacing it > with sg_map is not acceptable. What you seem to have missed is that sg_map is just a thin wrapper around kmap_atomic. Perhaps with a future check for a mappable page. This change should have zero impact on performance. Logan -- You received this message because you are subscribed to the Google Groups "open-iscsi" group. To unsubscribe from this group and stop receiving emails from it, send an email to open-iscsi+unsubscr...@googlegroups.com. To post to this group, send email to open-iscsi@googlegroups.com. Visit this group at https://groups.google.com/group/open-iscsi. For more options, visit https://groups.google.com/d/optout.
Re: [PATCH v2 01/21] scatterlist: Introduce sg_map helper functions
On 27/04/17 12:53 AM, Christoph Hellwig wrote: > I think you'll need to follow the existing kmap semantics and never > fail the iomem version either. Otherwise you'll have a special case > that's almost never used that has a different error path. > > Again, wrong way. Suddenly making things fail for your special case > that normally don't fail is a receipe for bugs. I don't disagree but these restrictions make the problem impossible to solve? If there is iomem behind a page in an SGL and someone tries to map it, we either have to fail or we break iomem safety which was your original concern. Logan -- You received this message because you are subscribed to the Google Groups "open-iscsi" group. To unsubscribe from this group and stop receiving emails from it, send an email to open-iscsi+unsubscr...@googlegroups.com. To post to this group, send email to open-iscsi@googlegroups.com. Visit this group at https://groups.google.com/group/open-iscsi. For more options, visit https://groups.google.com/d/optout.