On Thu, 20 Feb 2014, Mike Snitzer wrote: > On Thu, Feb 20 2014 at 7:10pm -0500, > Mikulas Patocka <mpatocka@xxxxxxxxxx> wrote: > > > On Thu, 20 Feb 2014, Mike Snitzer wrote: > > > > > On Thu, Feb 20 2014 at 6:01pm -0500, > > > Mikulas Patocka <mpatocka@xxxxxxxxxx> wrote: > > > > > > > Dm-crypt used per-cpu structures to hold pointers to ablkcipher_request. > > > > The code assumed that the work item keeps executing on a single CPU, so it > > > > used no synchronization when accessing this structure. > > > > > > > > When we disable a CPU by writing zero to > > > > /sys/devices/system/cpu/cpu*/online, the work item could be moved to > > > > another CPU. This causes crashes in dm-crypt because the code starts using > > > > a wrong ablkcipher_request. > > > > > > > > This patch fixes this bug by removing the percpu definition. The structure > > > > ablkcipher_request is accessed via a pointer from convert_context. > > > > Consequently, if the work item is rescheduled to a different CPU, the > > > > thread still uses the same ablkcipher_request. > > > > > > Hi Mikulas, > > > > > > Obviously avoiding crashes is more important than performance. > > > > > > But are we losing performance by switching away from using percpu? Do > > > we care? I'd like to see the header to speak to the potential for > > > slowdown (if there is any). > > > > There is one more allocation per request than before. I don't know how > > much does it cost. > > OK, any reason you didn't fix this up by using cpu hotplug hooks like > Tejun suggested? Too complicated? Yes, it would complicate the code. The patch that removes percpu pointers shortens the file, using cpu hotplug hooks would make it bigger. > > We could also modify the code to use per_bio_data to save one allocation. > > OK, sounds like a good win. Can you write a separate followup patch > that makes use of per_bio_data? I will try it. Mikulas -- dm-devel mailing list dm-devel@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/dm-devel
