Re: [PATCH 00/13] convert block layer to bioset_init()/mempool_init()
On Tue, May 22 2018 at 2:41am -0400, Christoph Hellwigwrote: > On Mon, May 21, 2018 at 07:38:55PM -0400, Kent Overstreet wrote: > > On Mon, May 21, 2018 at 02:24:32PM -0400, Mike Snitzer wrote: > > > Every single data structure change in this series should be reviewed for > > > unforeseen alignment consequences. Jens seemed to say that is > > > worthwhile. Not sure if he'll do it or we divide it up. If we divide > > > it up a temp topic branch should be published for others to inspect. > > > > > > Could be alignment hasn't been a historic concern for a bunch of the > > > data structures changed in this series.. if so then all we can do is fix > > > up any obvious potential for false sharing. > > > > Honestly, I almost never worry about alignment... the very few times I do > > care, > > I use __cacheline_aligned_in_smp. > > And that generally is the right stratgey. If Mike really doesn't want > a change we can just open code the kmalloc for the bio set there, the > important point is that we should not keep the old API around for no > good reason. Again, I never said I didn't want these changes. I just thought it worthwhile to mention the potential for alignment quirks arising. Reality is false sharing is quite uncommon. So my concern was/is pretty niche and unlikely to be applicable. That said, I did take the opportunity to look at all the DM structures that were modified. The bio_set and mempool_t structs are so large that they span multiple cachelines as is. So I focused on eliminating unnecessary spanning of cachelines (by non-bio_set and non-mempool_t members) and eliminated most holes in DM structs. This is the result: http://people.redhat.com/msnitzer/dm-4.18-struct-reorg/ Compare before.txt vs after_kent.txt vs after_mike.txt Nothing staggering or special. Just something I'll add once Kent's latest changes land. But, I also eliminated 2 4-byte holes in struct bio_set, Jens please feel free to pick this up (if you think it useful): From: Mike Snitzer Subject: [PATCH] block: eliminate 2 4-byte holes in bio_set structure Signed-off-by: Mike Snitzer --- include/linux/bio.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/include/linux/bio.h b/include/linux/bio.h index 5e472fc..e6fc692 100644 --- a/include/linux/bio.h +++ b/include/linux/bio.h @@ -735,7 +735,6 @@ static inline void bio_inc_remaining(struct bio *bio) struct bio_set { struct kmem_cache *bio_slab; - unsigned int front_pad; mempool_t bio_pool; mempool_t bvec_pool; @@ -743,6 +742,7 @@ struct bio_set { mempool_t bio_integrity_pool; mempool_t bvec_integrity_pool; #endif + unsigned int front_pad; /* * Deadlock avoidance for stacking block drivers: see comments in
Re: [PATCH 00/13] convert block layer to bioset_init()/mempool_init()
On Mon, May 21, 2018 at 07:38:55PM -0400, Kent Overstreet wrote: > On Mon, May 21, 2018 at 02:24:32PM -0400, Mike Snitzer wrote: > > Every single data structure change in this series should be reviewed for > > unforeseen alignment consequences. Jens seemed to say that is > > worthwhile. Not sure if he'll do it or we divide it up. If we divide > > it up a temp topic branch should be published for others to inspect. > > > > Could be alignment hasn't been a historic concern for a bunch of the > > data structures changed in this series.. if so then all we can do is fix > > up any obvious potential for false sharing. > > Honestly, I almost never worry about alignment... the very few times I do > care, > I use __cacheline_aligned_in_smp. And that generally is the right stratgey. If Mike really doesn't want a change we can just open code the kmalloc for the bio set there, the important point is that we should not keep the old API around for no good reason.
Re: [PATCH 00/13] convert block layer to bioset_init()/mempool_init()
On Mon, May 21, 2018 at 02:24:32PM -0400, Mike Snitzer wrote: > Every single data structure change in this series should be reviewed for > unforeseen alignment consequences. Jens seemed to say that is > worthwhile. Not sure if he'll do it or we divide it up. If we divide > it up a temp topic branch should be published for others to inspect. > > Could be alignment hasn't been a historic concern for a bunch of the > data structures changed in this series.. if so then all we can do is fix > up any obvious potential for false sharing. Honestly, I almost never worry about alignment... the very few times I do care, I use __cacheline_aligned_in_smp. If alignment is a concern in any of those structs, there really ought to be a comment indicating it. I very much doubt anything I touched was performance sensitive enough for it to be an issue, though. And if there is a performance impact, it should be oughtweighed by the reduced pointer chasing. If you disagree, I don't mind leaving the device mapper patch out, it really makes no difference to me. I could glance over for alignment issues but I feel like my analysis would not be terribly valuable to you considering I've already said my position on alignment is "meh, don't care" :)
Re: [PATCH 00/13] convert block layer to bioset_init()/mempool_init()
On Mon, May 21 2018 at 1:37pm -0400, Kent Overstreetwrote: > > Uh, you came across as upset and paranoid to me too. Chalk it up to email? :) Awesome. See how easy it is to make someone with purely constructive questions and feedback come off as upset and paranoid? The tipping point occurs when bait is set with: "It's not like ". Then: "Let's focus on getting it reviewed, rather than pontificate on what could potentially go all wrong with this." Message received: less pontificating, more doing! And here I thought that focusing on what could go wrong (across all code touched) was review. But OK, I'm the one that made this all weird ;) It is what it is at this point. > I personally don't care, I have no horse in this race. This particular patch > series wasn't my idea, Christoph wanted all these conversions done so > bioset_create() could be deleted. If you want us to hold off on the dm patch > for > awhile until someone can get around to testing it or whatever (since I don't > have tests for everything I pushed) that would be perfectly fine by me. As I clarified already: this isn't about DM. Every single data structure change in this series should be reviewed for unforeseen alignment consequences. Jens seemed to say that is worthwhile. Not sure if he'll do it or we divide it up. If we divide it up a temp topic branch should be published for others to inspect. Could be alignment hasn't been a historic concern for a bunch of the data structures changed in this series.. if so then all we can do is fix up any obvious potential for false sharing. Mike
Re: [PATCH 00/13] convert block layer to bioset_init()/mempool_init()
On Mon, May 21, 2018 at 12:09:14PM -0400, Mike Snitzer wrote: > On Mon, May 21 2018 at 11:36am -0400, > Jens Axboewrote: > > > On 5/21/18 9:18 AM, Mike Snitzer wrote: > > > On Mon, May 21 2018 at 11:09am -0400, > > > Jens Axboe wrote: > > > > > >> On 5/21/18 9:04 AM, Mike Snitzer wrote: > > >>> On Mon, May 21 2018 at 10:52am -0400, > > >>> Jens Axboe wrote: > > >>> > ... > > IMHO you're making a big deal out of something that should not be. > > >>> > > >>> I raised an issue that had seemingly not been considered at all. Not > > >>> making a big deal. Raising it for others' benefit. > > >>> > > If the dm bits are that sensitive and cache line honed to perfection > > already due to previous regressions in that area, then it might > > not be a bad idea to have some compile checks for false cacheline > > sharing between sensitive members, or spilling of a sub-struct > > into multiple cachelines. > > > > It's not like this was pushed behind your back. It's posted for > > review. It's quite possible the net change is a win for dm. Let's > > focus on getting it reviewed, rather than pontificate on what > > could potentially go all wrong with this. > > >>> > > >>> Why are you making this personal? Or purely about DM? I'm merely > > >>> pointing out this change isn't something that can be given a quick > > >>> blanket "looks good". > > >> > > >> I'm not making this personal at all?! You raised a (valid) concern, > > >> I'm merely stating why I don't think it's a high risk issue. I'm > > >> assuming your worry is related to dm, as those are the reports > > >> that would ultimately land on your desk. > > > > > > Then we'll just agree to disagree with what this implies: "It's not like > > > this was pushed behind your back." > > > > I'm afraid you've lost me now - it was not pushed behind your back, > > it was posted for review, with you on the CC list. Not trying to > > be deliberately dense here, I just don't see what our disagreement is. > > You're having an off day ;) Mondays and all? > > I just raised an alignment concern that needs to be considered during > review by all stakeholders. Wasn't upset at all. Maybe my email tone > came off otherwise? > > And then you got confused by me pointing out how it was weird for you to > suggest I felt this stuff was pushed behind my back.. and went on to > think I really think that. It's almost like you're a confused hypnotist > seeding me with paranoid dilutions. ;) Uh, you came across as upset and paranoid to me too. Chalk it up to email? :) I personally don't care, I have no horse in this race. This particular patch series wasn't my idea, Christoph wanted all these conversions done so bioset_create() could be deleted. If you want us to hold off on the dm patch for awhile until someone can get around to testing it or whatever (since I don't have tests for everything I pushed) that would be perfectly fine by me.
Re: [PATCH 00/13] convert block layer to bioset_init()/mempool_init()
On 5/21/18 10:09 AM, Mike Snitzer wrote: > On Mon, May 21 2018 at 11:36am -0400, > Jens Axboewrote: > >> On 5/21/18 9:18 AM, Mike Snitzer wrote: >>> On Mon, May 21 2018 at 11:09am -0400, >>> Jens Axboe wrote: >>> On 5/21/18 9:04 AM, Mike Snitzer wrote: > On Mon, May 21 2018 at 10:52am -0400, > Jens Axboe wrote: > > ... >> IMHO you're making a big deal out of something that should not be. > > I raised an issue that had seemingly not been considered at all. Not > making a big deal. Raising it for others' benefit. > >> If the dm bits are that sensitive and cache line honed to perfection >> already due to previous regressions in that area, then it might >> not be a bad idea to have some compile checks for false cacheline >> sharing between sensitive members, or spilling of a sub-struct >> into multiple cachelines. >> >> It's not like this was pushed behind your back. It's posted for >> review. It's quite possible the net change is a win for dm. Let's >> focus on getting it reviewed, rather than pontificate on what >> could potentially go all wrong with this. > > Why are you making this personal? Or purely about DM? I'm merely > pointing out this change isn't something that can be given a quick > blanket "looks good". I'm not making this personal at all?! You raised a (valid) concern, I'm merely stating why I don't think it's a high risk issue. I'm assuming your worry is related to dm, as those are the reports that would ultimately land on your desk. >>> >>> Then we'll just agree to disagree with what this implies: "It's not like >>> this was pushed behind your back." >> >> I'm afraid you've lost me now - it was not pushed behind your back, >> it was posted for review, with you on the CC list. Not trying to >> be deliberately dense here, I just don't see what our disagreement is. > > You're having an off day ;) Mondays and all? > > I just raised an alignment concern that needs to be considered during > review by all stakeholders. Wasn't upset at all. Maybe my email tone > came off otherwise? > > And then you got confused by me pointing out how it was weird for you to > suggest I felt this stuff was pushed behind my back.. and went on to > think I really think that. It's almost like you're a confused hypnotist > seeding me with paranoid dilutions. ;) > > /me waits for Jens to snap his fingers so he can just slowly step away > from this line of discussion that is solidly dead now... Mike, wtf are you talking about. >>> Reality is I'm fine with the change. Just think there is follow-on work >>> (now or later) that is needed. >> >> It's not hard to run the quick struct layout checks or alignment. If >> there's a concern, that should be done now, instead of being deferred to >> later. There's no point merging something that we expect to have >> follow-on work. If that's the case, then it should not be merged. There >> are no dependencies in the patchset, except that the last patch >> obviously can't be applied until all of the previous ones are in. > > Cool, sounds good. > >> I took a quick look at the struct mapped_device layout, which I'm >> assuming is the most important on the dm side. It's pretty big to >> begin with, obviously this makes it bigger since we're now >> embedding the bio_sets. On my config, doesn't show any false sharing >> that would be problematic, or layout changes that would worry me. FWIW. > > Great, thanks, do you happen to have a tree you can push so others can > get a quick compile and look at the series fully applied? I do not, I simply ran a quick analysis of the layout, then applied the full patchset, and repeated that exercise. Literally a 5 minute thing. I haven't applied the series, so haven't pushed anything out. > BTW, I'm upstream DM maintainer but I have a role in caring for related > subsystems (e.g. block core, etc) in downstream releases.. *cough* RHEL. > So this alignment concern wasn't ever purely about DM. I tend to care about that too... -- Jens Axboe
Re: [PATCH 00/13] convert block layer to bioset_init()/mempool_init()
On Mon, May 21 2018 at 11:36am -0400, Jens Axboewrote: > On 5/21/18 9:18 AM, Mike Snitzer wrote: > > On Mon, May 21 2018 at 11:09am -0400, > > Jens Axboe wrote: > > > >> On 5/21/18 9:04 AM, Mike Snitzer wrote: > >>> On Mon, May 21 2018 at 10:52am -0400, > >>> Jens Axboe wrote: > >>> ... > IMHO you're making a big deal out of something that should not be. > >>> > >>> I raised an issue that had seemingly not been considered at all. Not > >>> making a big deal. Raising it for others' benefit. > >>> > If the dm bits are that sensitive and cache line honed to perfection > already due to previous regressions in that area, then it might > not be a bad idea to have some compile checks for false cacheline > sharing between sensitive members, or spilling of a sub-struct > into multiple cachelines. > > It's not like this was pushed behind your back. It's posted for > review. It's quite possible the net change is a win for dm. Let's > focus on getting it reviewed, rather than pontificate on what > could potentially go all wrong with this. > >>> > >>> Why are you making this personal? Or purely about DM? I'm merely > >>> pointing out this change isn't something that can be given a quick > >>> blanket "looks good". > >> > >> I'm not making this personal at all?! You raised a (valid) concern, > >> I'm merely stating why I don't think it's a high risk issue. I'm > >> assuming your worry is related to dm, as those are the reports > >> that would ultimately land on your desk. > > > > Then we'll just agree to disagree with what this implies: "It's not like > > this was pushed behind your back." > > I'm afraid you've lost me now - it was not pushed behind your back, > it was posted for review, with you on the CC list. Not trying to > be deliberately dense here, I just don't see what our disagreement is. You're having an off day ;) Mondays and all? I just raised an alignment concern that needs to be considered during review by all stakeholders. Wasn't upset at all. Maybe my email tone came off otherwise? And then you got confused by me pointing out how it was weird for you to suggest I felt this stuff was pushed behind my back.. and went on to think I really think that. It's almost like you're a confused hypnotist seeding me with paranoid dilutions. ;) /me waits for Jens to snap his fingers so he can just slowly step away from this line of discussion that is solidly dead now... > > Reality is I'm fine with the change. Just think there is follow-on work > > (now or later) that is needed. > > It's not hard to run the quick struct layout checks or alignment. If > there's a concern, that should be done now, instead of being deferred to > later. There's no point merging something that we expect to have > follow-on work. If that's the case, then it should not be merged. There > are no dependencies in the patchset, except that the last patch > obviously can't be applied until all of the previous ones are in. Cool, sounds good. > I took a quick look at the struct mapped_device layout, which I'm > assuming is the most important on the dm side. It's pretty big to > begin with, obviously this makes it bigger since we're now > embedding the bio_sets. On my config, doesn't show any false sharing > that would be problematic, or layout changes that would worry me. FWIW. Great, thanks, do you happen to have a tree you can push so others can get a quick compile and look at the series fully applied? BTW, I'm upstream DM maintainer but I have a role in caring for related subsystems (e.g. block core, etc) in downstream releases.. *cough* RHEL. So this alignment concern wasn't ever purely about DM. Mike
Re: [PATCH 00/13] convert block layer to bioset_init()/mempool_init()
On 5/21/18 9:18 AM, Mike Snitzer wrote: > On Mon, May 21 2018 at 11:09am -0400, > Jens Axboewrote: > >> On 5/21/18 9:04 AM, Mike Snitzer wrote: >>> On Mon, May 21 2018 at 10:52am -0400, >>> Jens Axboe wrote: >>> On 5/21/18 8:47 AM, Mike Snitzer wrote: > On Mon, May 21 2018 at 10:36am -0400, > Jens Axboe wrote: > >> On 5/21/18 8:31 AM, Mike Snitzer wrote: >>> On Mon, May 21 2018 at 10:19am -0400, >>> Jens Axboe wrote: >>> On 5/21/18 8:03 AM, Mike Snitzer wrote: > On Sun, May 20 2018 at 6:25pm -0400, > Kent Overstreet wrote: > >> Jens - this series does the rest of the conversions that Christoph >> wanted, and >> drops bioset_create(). >> >> Only lightly tested, but the changes are pretty mechanical. Based on >> your >> for-next tree. > > By switching 'mempool_t *' to 'mempool_t' and 'bio_set *' to 'bio_set' > you've altered the alignment of members in data structures. So I'll > need to audit all the data structures you've modified in DM. > > Could we get the backstory on _why_ you're making this change? > Would go a long way to helping me appreciate why this is a good use of > anyone's time. Yeah, it's in the first series, it gets rid of a pointer indirection. >>> >>> "Allows mempools to be embedded in other structs, getting rid of a >>> pointer indirection from allocation fastpaths." >>> >>> So this is about using contiguous memory or avoiding partial allocation >>> failure? Or both? >>> >>> Or more to it? Just trying to fully appreciate the theory behind the >>> perceived associated benefit. >> >> It's about avoiding a pointer indirection. Instead of having to >> follow a pointer to get to that struct, it's simple offset math off >> your main structure. >> >>> I do think the increased risk of these embedded bio_set and mempool_t >>> themselves crossing cachelines, or struct members that follow them doing >>> so, really detracts from these types of changes. >> >> Definitely something to look out for, though most of them should be >> per-dev structures and not in-flight structures. That makes it a bit >> less sensitive. But can't hurt to audit the layouts and adjust if >> necessary. This is why it's posted for review :-) > > This isn't something that is easily caught upfront. Yes we can all be > busy little beavers with pahole to audit alignment. But chances are > most people won't do it. > > Reality is there is potential for a regression due to false sharing to > creep in if a hot struct member suddenly starts straddling a cacheline. > That type of NUMA performance killer is pretty insidious and somewhat > tedious to hunt down even when looking for it with specialized tools: > https://joemario.github.io/blog/2016/09/01/c2c-blog/ IMHO you're making a big deal out of something that should not be. >>> >>> I raised an issue that had seemingly not been considered at all. Not >>> making a big deal. Raising it for others' benefit. >>> If the dm bits are that sensitive and cache line honed to perfection already due to previous regressions in that area, then it might not be a bad idea to have some compile checks for false cacheline sharing between sensitive members, or spilling of a sub-struct into multiple cachelines. It's not like this was pushed behind your back. It's posted for review. It's quite possible the net change is a win for dm. Let's focus on getting it reviewed, rather than pontificate on what could potentially go all wrong with this. >>> >>> Why are you making this personal? Or purely about DM? I'm merely >>> pointing out this change isn't something that can be given a quick >>> blanket "looks good". >> >> I'm not making this personal at all?! You raised a (valid) concern, >> I'm merely stating why I don't think it's a high risk issue. I'm >> assuming your worry is related to dm, as those are the reports >> that would ultimately land on your desk. > > Then we'll just agree to disagree with what this implies: "It's not like > this was pushed behind your back." I'm afraid you've lost me now - it was not pushed behind your back, it was posted for review, with you on the CC list. Not trying to be deliberately dense here, I just don't see what our disagreement is. > Reality is I'm fine with the change. Just think there is follow-on work > (now or later) that is needed. It's not hard to run the quick struct layout checks or alignment. If there's a concern, that should be done now, instead of being deferred to later. There's no point merging something that we expect to have follow-on work. If that's the
Re: [PATCH 00/13] convert block layer to bioset_init()/mempool_init()
On 5/21/18 9:12 AM, David Sterba wrote: > On Mon, May 21, 2018 at 08:19:58AM -0600, Jens Axboe wrote: >> On 5/21/18 8:03 AM, Mike Snitzer wrote: >>> On Sun, May 20 2018 at 6:25pm -0400, >>> Kent Overstreetwrote: >>> Jens - this series does the rest of the conversions that Christoph wanted, and drops bioset_create(). Only lightly tested, but the changes are pretty mechanical. Based on your for-next tree. >>> >>> By switching 'mempool_t *' to 'mempool_t' and 'bio_set *' to 'bio_set' >>> you've altered the alignment of members in data structures. So I'll >>> need to audit all the data structures you've modified in DM. >>> >>> Could we get the backstory on _why_ you're making this change? >>> Would go a long way to helping me appreciate why this is a good use of >>> anyone's time. >> >> Yeah, it's in the first series, it gets rid of a pointer indirection. > > This should to be also mentioned the changelog of each patch. There are > 12 subsystems changed, this could be about 10 maintainers and I guess > everybody has the same question why the change is made. Agree, the justification should be in this series as well, of course. Kent, might not be a bad idea to resend with a more descriptive cover letter. -- Jens Axboe
Re: [PATCH 00/13] convert block layer to bioset_init()/mempool_init()
On Mon, May 21 2018 at 11:09am -0400, Jens Axboewrote: > On 5/21/18 9:04 AM, Mike Snitzer wrote: > > On Mon, May 21 2018 at 10:52am -0400, > > Jens Axboe wrote: > > > >> On 5/21/18 8:47 AM, Mike Snitzer wrote: > >>> On Mon, May 21 2018 at 10:36am -0400, > >>> Jens Axboe wrote: > >>> > On 5/21/18 8:31 AM, Mike Snitzer wrote: > > On Mon, May 21 2018 at 10:19am -0400, > > Jens Axboe wrote: > > > >> On 5/21/18 8:03 AM, Mike Snitzer wrote: > >>> On Sun, May 20 2018 at 6:25pm -0400, > >>> Kent Overstreet wrote: > >>> > Jens - this series does the rest of the conversions that Christoph > wanted, and > drops bioset_create(). > > Only lightly tested, but the changes are pretty mechanical. Based on > your > for-next tree. > >>> > >>> By switching 'mempool_t *' to 'mempool_t' and 'bio_set *' to 'bio_set' > >>> you've altered the alignment of members in data structures. So I'll > >>> need to audit all the data structures you've modified in DM. > >>> > >>> Could we get the backstory on _why_ you're making this change? > >>> Would go a long way to helping me appreciate why this is a good use of > >>> anyone's time. > >> > >> Yeah, it's in the first series, it gets rid of a pointer indirection. > > > > "Allows mempools to be embedded in other structs, getting rid of a > > pointer indirection from allocation fastpaths." > > > > So this is about using contiguous memory or avoiding partial allocation > > failure? Or both? > > > > Or more to it? Just trying to fully appreciate the theory behind the > > perceived associated benefit. > > It's about avoiding a pointer indirection. Instead of having to > follow a pointer to get to that struct, it's simple offset math off > your main structure. > > > I do think the increased risk of these embedded bio_set and mempool_t > > themselves crossing cachelines, or struct members that follow them doing > > so, really detracts from these types of changes. > > Definitely something to look out for, though most of them should be > per-dev structures and not in-flight structures. That makes it a bit > less sensitive. But can't hurt to audit the layouts and adjust if > necessary. This is why it's posted for review :-) > >>> > >>> This isn't something that is easily caught upfront. Yes we can all be > >>> busy little beavers with pahole to audit alignment. But chances are > >>> most people won't do it. > >>> > >>> Reality is there is potential for a regression due to false sharing to > >>> creep in if a hot struct member suddenly starts straddling a cacheline. > >>> That type of NUMA performance killer is pretty insidious and somewhat > >>> tedious to hunt down even when looking for it with specialized tools: > >>> https://joemario.github.io/blog/2016/09/01/c2c-blog/ > >> > >> IMHO you're making a big deal out of something that should not be. > > > > I raised an issue that had seemingly not been considered at all. Not > > making a big deal. Raising it for others' benefit. > > > >> If the dm bits are that sensitive and cache line honed to perfection > >> already due to previous regressions in that area, then it might > >> not be a bad idea to have some compile checks for false cacheline > >> sharing between sensitive members, or spilling of a sub-struct > >> into multiple cachelines. > >> > >> It's not like this was pushed behind your back. It's posted for > >> review. It's quite possible the net change is a win for dm. Let's > >> focus on getting it reviewed, rather than pontificate on what > >> could potentially go all wrong with this. > > > > Why are you making this personal? Or purely about DM? I'm merely > > pointing out this change isn't something that can be given a quick > > blanket "looks good". > > I'm not making this personal at all?! You raised a (valid) concern, > I'm merely stating why I don't think it's a high risk issue. I'm > assuming your worry is related to dm, as those are the reports > that would ultimately land on your desk. Then we'll just agree to disagree with what this implies: "It's not like this was pushed behind your back." Reality is I'm fine with the change. Just think there is follow-on work (now or later) that is needed. Enough said.
Re: [PATCH 00/13] convert block layer to bioset_init()/mempool_init()
On Mon, May 21, 2018 at 08:19:58AM -0600, Jens Axboe wrote: > On 5/21/18 8:03 AM, Mike Snitzer wrote: > > On Sun, May 20 2018 at 6:25pm -0400, > > Kent Overstreetwrote: > > > >> Jens - this series does the rest of the conversions that Christoph wanted, > >> and > >> drops bioset_create(). > >> > >> Only lightly tested, but the changes are pretty mechanical. Based on your > >> for-next tree. > > > > By switching 'mempool_t *' to 'mempool_t' and 'bio_set *' to 'bio_set' > > you've altered the alignment of members in data structures. So I'll > > need to audit all the data structures you've modified in DM. > > > > Could we get the backstory on _why_ you're making this change? > > Would go a long way to helping me appreciate why this is a good use of > > anyone's time. > > Yeah, it's in the first series, it gets rid of a pointer indirection. This should to be also mentioned the changelog of each patch. There are 12 subsystems changed, this could be about 10 maintainers and I guess everybody has the same question why the change is made. The conversion is not exactly the same in all patches, the simple pointer -> static variable can be possibly covered by the same generic text but as Mike points out the alignment changes should be at least mentioned for consideration otherwise.
Re: [PATCH 00/13] convert block layer to bioset_init()/mempool_init()
On 5/21/18 9:04 AM, Mike Snitzer wrote: > On Mon, May 21 2018 at 10:52am -0400, > Jens Axboewrote: > >> On 5/21/18 8:47 AM, Mike Snitzer wrote: >>> On Mon, May 21 2018 at 10:36am -0400, >>> Jens Axboe wrote: >>> On 5/21/18 8:31 AM, Mike Snitzer wrote: > On Mon, May 21 2018 at 10:19am -0400, > Jens Axboe wrote: > >> On 5/21/18 8:03 AM, Mike Snitzer wrote: >>> On Sun, May 20 2018 at 6:25pm -0400, >>> Kent Overstreet wrote: >>> Jens - this series does the rest of the conversions that Christoph wanted, and drops bioset_create(). Only lightly tested, but the changes are pretty mechanical. Based on your for-next tree. >>> >>> By switching 'mempool_t *' to 'mempool_t' and 'bio_set *' to 'bio_set' >>> you've altered the alignment of members in data structures. So I'll >>> need to audit all the data structures you've modified in DM. >>> >>> Could we get the backstory on _why_ you're making this change? >>> Would go a long way to helping me appreciate why this is a good use of >>> anyone's time. >> >> Yeah, it's in the first series, it gets rid of a pointer indirection. > > "Allows mempools to be embedded in other structs, getting rid of a > pointer indirection from allocation fastpaths." > > So this is about using contiguous memory or avoiding partial allocation > failure? Or both? > > Or more to it? Just trying to fully appreciate the theory behind the > perceived associated benefit. It's about avoiding a pointer indirection. Instead of having to follow a pointer to get to that struct, it's simple offset math off your main structure. > I do think the increased risk of these embedded bio_set and mempool_t > themselves crossing cachelines, or struct members that follow them doing > so, really detracts from these types of changes. Definitely something to look out for, though most of them should be per-dev structures and not in-flight structures. That makes it a bit less sensitive. But can't hurt to audit the layouts and adjust if necessary. This is why it's posted for review :-) >>> >>> This isn't something that is easily caught upfront. Yes we can all be >>> busy little beavers with pahole to audit alignment. But chances are >>> most people won't do it. >>> >>> Reality is there is potential for a regression due to false sharing to >>> creep in if a hot struct member suddenly starts straddling a cacheline. >>> That type of NUMA performance killer is pretty insidious and somewhat >>> tedious to hunt down even when looking for it with specialized tools: >>> https://joemario.github.io/blog/2016/09/01/c2c-blog/ >> >> IMHO you're making a big deal out of something that should not be. > > I raised an issue that had seemingly not been considered at all. Not > making a big deal. Raising it for others' benefit. > >> If the dm bits are that sensitive and cache line honed to perfection >> already due to previous regressions in that area, then it might >> not be a bad idea to have some compile checks for false cacheline >> sharing between sensitive members, or spilling of a sub-struct >> into multiple cachelines. >> >> It's not like this was pushed behind your back. It's posted for >> review. It's quite possible the net change is a win for dm. Let's >> focus on getting it reviewed, rather than pontificate on what >> could potentially go all wrong with this. > > Why are you making this personal? Or purely about DM? I'm merely > pointing out this change isn't something that can be given a quick > blanket "looks good". I'm not making this personal at all?! You raised a (valid) concern, I'm merely stating why I don't think it's a high risk issue. I'm assuming your worry is related to dm, as those are the reports that would ultimately land on your desk. -- Jens Axboe
Re: [PATCH 00/13] convert block layer to bioset_init()/mempool_init()
On Mon, May 21 2018 at 10:52am -0400, Jens Axboewrote: > On 5/21/18 8:47 AM, Mike Snitzer wrote: > > On Mon, May 21 2018 at 10:36am -0400, > > Jens Axboe wrote: > > > >> On 5/21/18 8:31 AM, Mike Snitzer wrote: > >>> On Mon, May 21 2018 at 10:19am -0400, > >>> Jens Axboe wrote: > >>> > On 5/21/18 8:03 AM, Mike Snitzer wrote: > > On Sun, May 20 2018 at 6:25pm -0400, > > Kent Overstreet wrote: > > > >> Jens - this series does the rest of the conversions that Christoph > >> wanted, and > >> drops bioset_create(). > >> > >> Only lightly tested, but the changes are pretty mechanical. Based on > >> your > >> for-next tree. > > > > By switching 'mempool_t *' to 'mempool_t' and 'bio_set *' to 'bio_set' > > you've altered the alignment of members in data structures. So I'll > > need to audit all the data structures you've modified in DM. > > > > Could we get the backstory on _why_ you're making this change? > > Would go a long way to helping me appreciate why this is a good use of > > anyone's time. > > Yeah, it's in the first series, it gets rid of a pointer indirection. > >>> > >>> "Allows mempools to be embedded in other structs, getting rid of a > >>> pointer indirection from allocation fastpaths." > >>> > >>> So this is about using contiguous memory or avoiding partial allocation > >>> failure? Or both? > >>> > >>> Or more to it? Just trying to fully appreciate the theory behind the > >>> perceived associated benefit. > >> > >> It's about avoiding a pointer indirection. Instead of having to > >> follow a pointer to get to that struct, it's simple offset math off > >> your main structure. > >> > >>> I do think the increased risk of these embedded bio_set and mempool_t > >>> themselves crossing cachelines, or struct members that follow them doing > >>> so, really detracts from these types of changes. > >> > >> Definitely something to look out for, though most of them should be > >> per-dev structures and not in-flight structures. That makes it a bit > >> less sensitive. But can't hurt to audit the layouts and adjust if > >> necessary. This is why it's posted for review :-) > > > > This isn't something that is easily caught upfront. Yes we can all be > > busy little beavers with pahole to audit alignment. But chances are > > most people won't do it. > > > > Reality is there is potential for a regression due to false sharing to > > creep in if a hot struct member suddenly starts straddling a cacheline. > > That type of NUMA performance killer is pretty insidious and somewhat > > tedious to hunt down even when looking for it with specialized tools: > > https://joemario.github.io/blog/2016/09/01/c2c-blog/ > > IMHO you're making a big deal out of something that should not be. I raised an issue that had seemingly not been considered at all. Not making a big deal. Raising it for others' benefit. > If the dm bits are that sensitive and cache line honed to perfection > already due to previous regressions in that area, then it might > not be a bad idea to have some compile checks for false cacheline > sharing between sensitive members, or spilling of a sub-struct > into multiple cachelines. > > It's not like this was pushed behind your back. It's posted for > review. It's quite possible the net change is a win for dm. Let's > focus on getting it reviewed, rather than pontificate on what > could potentially go all wrong with this. Why are you making this personal? Or purely about DM? I'm merely pointing out this change isn't something that can be given a quick blanket "looks good". Mike
Re: [PATCH 00/13] convert block layer to bioset_init()/mempool_init()
On 5/21/18 8:47 AM, Mike Snitzer wrote: > On Mon, May 21 2018 at 10:36am -0400, > Jens Axboewrote: > >> On 5/21/18 8:31 AM, Mike Snitzer wrote: >>> On Mon, May 21 2018 at 10:19am -0400, >>> Jens Axboe wrote: >>> On 5/21/18 8:03 AM, Mike Snitzer wrote: > On Sun, May 20 2018 at 6:25pm -0400, > Kent Overstreet wrote: > >> Jens - this series does the rest of the conversions that Christoph >> wanted, and >> drops bioset_create(). >> >> Only lightly tested, but the changes are pretty mechanical. Based on your >> for-next tree. > > By switching 'mempool_t *' to 'mempool_t' and 'bio_set *' to 'bio_set' > you've altered the alignment of members in data structures. So I'll > need to audit all the data structures you've modified in DM. > > Could we get the backstory on _why_ you're making this change? > Would go a long way to helping me appreciate why this is a good use of > anyone's time. Yeah, it's in the first series, it gets rid of a pointer indirection. >>> >>> "Allows mempools to be embedded in other structs, getting rid of a >>> pointer indirection from allocation fastpaths." >>> >>> So this is about using contiguous memory or avoiding partial allocation >>> failure? Or both? >>> >>> Or more to it? Just trying to fully appreciate the theory behind the >>> perceived associated benefit. >> >> It's about avoiding a pointer indirection. Instead of having to >> follow a pointer to get to that struct, it's simple offset math off >> your main structure. >> >>> I do think the increased risk of these embedded bio_set and mempool_t >>> themselves crossing cachelines, or struct members that follow them doing >>> so, really detracts from these types of changes. >> >> Definitely something to look out for, though most of them should be >> per-dev structures and not in-flight structures. That makes it a bit >> less sensitive. But can't hurt to audit the layouts and adjust if >> necessary. This is why it's posted for review :-) > > This isn't something that is easily caught upfront. Yes we can all be > busy little beavers with pahole to audit alignment. But chances are > most people won't do it. > > Reality is there is potential for a regression due to false sharing to > creep in if a hot struct member suddenly starts straddling a cacheline. > That type of NUMA performance killer is pretty insidious and somewhat > tedious to hunt down even when looking for it with specialized tools: > https://joemario.github.io/blog/2016/09/01/c2c-blog/ IMHO you're making a big deal out of something that should not be. If the dm bits are that sensitive and cache line honed to perfection already due to previous regressions in that area, then it might not be a bad idea to have some compile checks for false cacheline sharing between sensitive members, or spilling of a sub-struct into multiple cachelines. It's not like this was pushed behind your back. It's posted for review. It's quite possible the net change is a win for dm. Let's focus on getting it reviewed, rather than pontificate on what could potentially go all wrong with this. -- Jens Axboe
Re: [PATCH 00/13] convert block layer to bioset_init()/mempool_init()
On Mon, May 21 2018 at 10:36am -0400, Jens Axboewrote: > On 5/21/18 8:31 AM, Mike Snitzer wrote: > > On Mon, May 21 2018 at 10:19am -0400, > > Jens Axboe wrote: > > > >> On 5/21/18 8:03 AM, Mike Snitzer wrote: > >>> On Sun, May 20 2018 at 6:25pm -0400, > >>> Kent Overstreet wrote: > >>> > Jens - this series does the rest of the conversions that Christoph > wanted, and > drops bioset_create(). > > Only lightly tested, but the changes are pretty mechanical. Based on your > for-next tree. > >>> > >>> By switching 'mempool_t *' to 'mempool_t' and 'bio_set *' to 'bio_set' > >>> you've altered the alignment of members in data structures. So I'll > >>> need to audit all the data structures you've modified in DM. > >>> > >>> Could we get the backstory on _why_ you're making this change? > >>> Would go a long way to helping me appreciate why this is a good use of > >>> anyone's time. > >> > >> Yeah, it's in the first series, it gets rid of a pointer indirection. > > > > "Allows mempools to be embedded in other structs, getting rid of a > > pointer indirection from allocation fastpaths." > > > > So this is about using contiguous memory or avoiding partial allocation > > failure? Or both? > > > > Or more to it? Just trying to fully appreciate the theory behind the > > perceived associated benefit. > > It's about avoiding a pointer indirection. Instead of having to > follow a pointer to get to that struct, it's simple offset math off > your main structure. > > > I do think the increased risk of these embedded bio_set and mempool_t > > themselves crossing cachelines, or struct members that follow them doing > > so, really detracts from these types of changes. > > Definitely something to look out for, though most of them should be > per-dev structures and not in-flight structures. That makes it a bit > less sensitive. But can't hurt to audit the layouts and adjust if > necessary. This is why it's posted for review :-) This isn't something that is easily caught upfront. Yes we can all be busy little beavers with pahole to audit alignment. But chances are most people won't do it. Reality is there is potential for a regression due to false sharing to creep in if a hot struct member suddenly starts straddling a cacheline. That type of NUMA performance killer is pretty insidious and somewhat tedious to hunt down even when looking for it with specialized tools: https://joemario.github.io/blog/2016/09/01/c2c-blog/
Re: [PATCH 00/13] convert block layer to bioset_init()/mempool_init()
On 5/21/18 8:31 AM, Mike Snitzer wrote: > On Mon, May 21 2018 at 10:19am -0400, > Jens Axboewrote: > >> On 5/21/18 8:03 AM, Mike Snitzer wrote: >>> On Sun, May 20 2018 at 6:25pm -0400, >>> Kent Overstreet wrote: >>> Jens - this series does the rest of the conversions that Christoph wanted, and drops bioset_create(). Only lightly tested, but the changes are pretty mechanical. Based on your for-next tree. >>> >>> By switching 'mempool_t *' to 'mempool_t' and 'bio_set *' to 'bio_set' >>> you've altered the alignment of members in data structures. So I'll >>> need to audit all the data structures you've modified in DM. >>> >>> Could we get the backstory on _why_ you're making this change? >>> Would go a long way to helping me appreciate why this is a good use of >>> anyone's time. >> >> Yeah, it's in the first series, it gets rid of a pointer indirection. > > "Allows mempools to be embedded in other structs, getting rid of a > pointer indirection from allocation fastpaths." > > So this is about using contiguous memory or avoiding partial allocation > failure? Or both? > > Or more to it? Just trying to fully appreciate the theory behind the > perceived associated benefit. It's about avoiding a pointer indirection. Instead of having to follow a pointer to get to that struct, it's simple offset math off your main structure. > I do think the increased risk of these embedded bio_set and mempool_t > themselves crossing cachelines, or struct members that follow them doing > so, really detracts from these types of changes. Definitely something to look out for, though most of them should be per-dev structures and not in-flight structures. That makes it a bit less sensitive. But can't hurt to audit the layouts and adjust if necessary. This is why it's posted for review :-) -- Jens Axboe
Re: [PATCH 00/13] convert block layer to bioset_init()/mempool_init()
On Mon, May 21 2018 at 10:19am -0400, Jens Axboewrote: > On 5/21/18 8:03 AM, Mike Snitzer wrote: > > On Sun, May 20 2018 at 6:25pm -0400, > > Kent Overstreet wrote: > > > >> Jens - this series does the rest of the conversions that Christoph wanted, > >> and > >> drops bioset_create(). > >> > >> Only lightly tested, but the changes are pretty mechanical. Based on your > >> for-next tree. > > > > By switching 'mempool_t *' to 'mempool_t' and 'bio_set *' to 'bio_set' > > you've altered the alignment of members in data structures. So I'll > > need to audit all the data structures you've modified in DM. > > > > Could we get the backstory on _why_ you're making this change? > > Would go a long way to helping me appreciate why this is a good use of > > anyone's time. > > Yeah, it's in the first series, it gets rid of a pointer indirection. "Allows mempools to be embedded in other structs, getting rid of a pointer indirection from allocation fastpaths." So this is about using contiguous memory or avoiding partial allocation failure? Or both? Or more to it? Just trying to fully appreciate the theory behind the perceived associated benefit. I do think the increased risk of these embedded bio_set and mempool_t themselves crossing cachelines, or struct members that follow them doing so, really detracts from these types of changes. Thanks, Mike
Re: [PATCH 00/13] convert block layer to bioset_init()/mempool_init()
On 5/20/18 4:25 PM, Kent Overstreet wrote: > Jens - this series does the rest of the conversions that Christoph wanted, and > drops bioset_create(). > > Only lightly tested, but the changes are pretty mechanical. Based on your > for-next tree. Looks good to me. I'll let it simmer for a bit to give folks a chance to comment on it. -- Jens Axboe
Re: [PATCH 00/13] convert block layer to bioset_init()/mempool_init()
On 5/21/18 8:03 AM, Mike Snitzer wrote: > On Sun, May 20 2018 at 6:25pm -0400, > Kent Overstreetwrote: > >> Jens - this series does the rest of the conversions that Christoph wanted, >> and >> drops bioset_create(). >> >> Only lightly tested, but the changes are pretty mechanical. Based on your >> for-next tree. > > By switching 'mempool_t *' to 'mempool_t' and 'bio_set *' to 'bio_set' > you've altered the alignment of members in data structures. So I'll > need to audit all the data structures you've modified in DM. > > Could we get the backstory on _why_ you're making this change? > Would go a long way to helping me appreciate why this is a good use of > anyone's time. Yeah, it's in the first series, it gets rid of a pointer indirection. -- Jens Axboe
Re: [PATCH 00/13] convert block layer to bioset_init()/mempool_init()
On Sun, May 20 2018 at 6:25pm -0400, Kent Overstreetwrote: > Jens - this series does the rest of the conversions that Christoph wanted, and > drops bioset_create(). > > Only lightly tested, but the changes are pretty mechanical. Based on your > for-next tree. By switching 'mempool_t *' to 'mempool_t' and 'bio_set *' to 'bio_set' you've altered the alignment of members in data structures. So I'll need to audit all the data structures you've modified in DM. Could we get the backstory on _why_ you're making this change? Would go a long way to helping me appreciate why this is a good use of anyone's time. Thanks, Mike
Re: [PATCH 00/13] convert block layer to bioset_init()/mempool_init()
Thanks - sending it to him On Sun, May 20, 2018 at 7:08 PM, NeilBrownwrote: > On Sun, May 20 2018, Kent Overstreet wrote: > >> Jens - this series does the rest of the conversions that Christoph wanted, >> and >> drops bioset_create(). >> >> Only lightly tested, but the changes are pretty mechanical. Based on your >> for-next tree. >> >> It's also in the for-jens branch at >> https://evilpiepirate.org/git/bcachefs.git >> >> Kent Overstreet (12): >> block: convert bounce, q->bio_split to bioset_init()/mempool_init() >> drbd: convert to bioset_init()/mempool_init() >> pktcdvd: convert to bioset_init()/mempool_init() >> lightnvm: convert to bioset_init()/mempool_init() >> bcache: convert to bioset_init()/mempool_init() >> md: convert to bioset_init()/mempool_init() > > Hi Kent, > this conversion looks really good, thanks for Ccing me on it. > However as Shaohua Li is now the maintainer of md, it probably should > have gone to him as well. > > Thanks, > NeilBrown
Re: [PATCH 00/13] convert block layer to bioset_init()/mempool_init()
On Sun, May 20 2018, Kent Overstreet wrote: > Jens - this series does the rest of the conversions that Christoph wanted, and > drops bioset_create(). > > Only lightly tested, but the changes are pretty mechanical. Based on your > for-next tree. > > It's also in the for-jens branch at https://evilpiepirate.org/git/bcachefs.git > > Kent Overstreet (12): > block: convert bounce, q->bio_split to bioset_init()/mempool_init() > drbd: convert to bioset_init()/mempool_init() > pktcdvd: convert to bioset_init()/mempool_init() > lightnvm: convert to bioset_init()/mempool_init() > bcache: convert to bioset_init()/mempool_init() > md: convert to bioset_init()/mempool_init() Hi Kent, this conversion looks really good, thanks for Ccing me on it. However as Shaohua Li is now the maintainer of md, it probably should have gone to him as well. Thanks, NeilBrown signature.asc Description: PGP signature
[PATCH 00/13] convert block layer to bioset_init()/mempool_init()
Jens - this series does the rest of the conversions that Christoph wanted, and drops bioset_create(). Only lightly tested, but the changes are pretty mechanical. Based on your for-next tree. It's also in the for-jens branch at https://evilpiepirate.org/git/bcachefs.git Kent Overstreet (12): block: convert bounce, q->bio_split to bioset_init()/mempool_init() drbd: convert to bioset_init()/mempool_init() pktcdvd: convert to bioset_init()/mempool_init() lightnvm: convert to bioset_init()/mempool_init() bcache: convert to bioset_init()/mempool_init() md: convert to bioset_init()/mempool_init() dm: convert to bioset_init()/mempool_init() target: convert to bioset_init()/mempool_init() fs: convert block_dev.c to bioset_init() btrfs: convert to bioset_init()/mempool_init() xfs: convert to bioset_init()/mempool_init() block: Drop bioset_create() block/bio.c | 61 +-- block/blk-core.c| 7 +-- block/blk-merge.c | 8 +-- block/blk-sysfs.c | 3 +- block/bounce.c | 47 +- drivers/block/drbd/drbd_bitmap.c| 4 +- drivers/block/drbd/drbd_int.h | 10 ++-- drivers/block/drbd/drbd_main.c | 71 ++- drivers/block/drbd/drbd_receiver.c | 6 +-- drivers/block/drbd/drbd_req.c | 4 +- drivers/block/drbd/drbd_req.h | 2 +- drivers/block/pktcdvd.c | 50 +-- drivers/lightnvm/pblk-core.c| 30 ++-- drivers/lightnvm/pblk-init.c| 72 +-- drivers/lightnvm/pblk-read.c| 4 +- drivers/lightnvm/pblk-recovery.c| 2 +- drivers/lightnvm/pblk-write.c | 8 +-- drivers/lightnvm/pblk.h | 14 +++--- drivers/md/bcache/bcache.h | 10 ++-- drivers/md/bcache/bset.c| 13 ++--- drivers/md/bcache/bset.h| 2 +- drivers/md/bcache/btree.c | 4 +- drivers/md/bcache/io.c | 4 +- drivers/md/bcache/request.c | 18 +++ drivers/md/bcache/super.c | 38 ++- drivers/md/dm-bio-prison-v1.c | 13 ++--- drivers/md/dm-bio-prison-v2.c | 13 ++--- drivers/md/dm-cache-target.c| 25 +- drivers/md/dm-core.h| 4 +- drivers/md/dm-crypt.c | 60 +++ drivers/md/dm-integrity.c | 15 +++--- drivers/md/dm-io.c | 29 +-- drivers/md/dm-kcopyd.c | 22 + drivers/md/dm-log-userspace-base.c | 19 drivers/md/dm-region-hash.c | 23 - drivers/md/dm-rq.c | 2 +- drivers/md/dm-snap.c| 17 +++ drivers/md/dm-thin.c| 32 ++-- drivers/md/dm-verity-fec.c | 55 +++-- drivers/md/dm-verity-fec.h | 8 +-- drivers/md/dm-zoned-target.c| 13 +++-- drivers/md/dm.c | 55 + drivers/md/md-faulty.c | 2 +- drivers/md/md-linear.c | 2 +- drivers/md/md-multipath.c | 17 --- drivers/md/md-multipath.h | 2 +- drivers/md/md.c | 61 +-- drivers/md/md.h | 4 +- drivers/md/raid0.c | 5 +- drivers/md/raid1.c | 76 ++--- drivers/md/raid1.h | 6 +-- drivers/md/raid10.c | 60 +++ drivers/md/raid10.h | 6 +-- drivers/md/raid5-cache.c| 43 drivers/md/raid5-ppl.c | 42 +++- drivers/md/raid5.c | 12 ++--- drivers/md/raid5.h | 2 +- drivers/target/target_core_iblock.c | 14 +++--- drivers/target/target_core_iblock.h | 2 +- fs/block_dev.c | 9 ++-- fs/btrfs/extent_io.c| 25 +- fs/xfs/xfs_aops.c | 2 +- fs/xfs/xfs_aops.h | 2 +- fs/xfs/xfs_super.c | 11 ++--- include/linux/bio.h | 11 +++-- include/linux/blkdev.h | 2 +- include/linux/pktcdvd.h | 2 +- 67 files changed, 606 insertions(+), 711 deletions(-) -- 2.17.0