From patchwork Tue Sep 20 22:52:27 2022 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Peter Xu X-Patchwork-Id: 1680423 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@legolas.ozlabs.org Authentication-Results: legolas.ozlabs.org; spf=pass (sender SPF authorized) smtp.mailfrom=nongnu.org (client-ip=209.51.188.17; helo=lists.gnu.org; envelope-from=qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org; receiver=) Authentication-Results: legolas.ozlabs.org; dkim=pass (1024-bit key; unprotected) header.d=redhat.com header.i=@redhat.com header.a=rsa-sha256 header.s=mimecast20190719 header.b=icb2oiMS; dkim-atps=neutral Received: from lists.gnu.org (lists.gnu.org [209.51.188.17]) (using TLSv1.2 with cipher ECDHE-ECDSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by legolas.ozlabs.org (Postfix) with ESMTPS id 4MXJv05mnHz1ypH for ; Wed, 21 Sep 2022 10:19:24 +1000 (AEST) Received: from localhost ([::1]:41488 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1oanSU-0003Fg-Bz for incoming@patchwork.ozlabs.org; Tue, 20 Sep 2022 20:19:22 -0400 Received: from eggs.gnu.org ([2001:470:142:3::10]:33456) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1oam6V-0005zo-3Z for qemu-devel@nongnu.org; Tue, 20 Sep 2022 18:52:35 -0400 Received: from us-smtp-delivery-124.mimecast.com ([170.10.129.124]:35243) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1oam6S-0001VP-Ir for qemu-devel@nongnu.org; Tue, 20 Sep 2022 18:52:34 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1663714352; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=eFM6UBQq9Gr7xD4s6za7TeZ1aBDahyr8gEmlZoCT4PI=; b=icb2oiMSZVvAubtk6JLf0pxssARoFKlVGsTe/V5BqfFlPKM+Pa7jq1740pN0oTYMTk6YUo ZBt4XbVRugpoZOOqPjTxWU+Pi+e/qTl5wz3ac94RA5OQft5EEAhR+7n4R3jTz9ZXi2tvgx IuqS3+sbEU4r1CSc3K5N0SRx/+rIITU= Received: from mail-qv1-f69.google.com (mail-qv1-f69.google.com [209.85.219.69]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_128_GCM_SHA256) id us-mta-85-mDsLSB9aM-ugoO9R3YQddg-1; Tue, 20 Sep 2022 18:52:31 -0400 X-MC-Unique: mDsLSB9aM-ugoO9R3YQddg-1 Received: by mail-qv1-f69.google.com with SMTP id n15-20020ad444af000000b004a2a341ad71so3049218qvt.15 for ; Tue, 20 Sep 2022 15:52:30 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:cc:to:from:x-gm-message-state:from:to:cc :subject:date; bh=eFM6UBQq9Gr7xD4s6za7TeZ1aBDahyr8gEmlZoCT4PI=; b=S+TJaucxs+Pa4q7n1DhgloPXF7dcDGM50IwYIStPJpFYdZDcxj5CEr7+oSyWqgzqel U7dKWcQvlrSKXTSexX39+mXrh/qn/KM3vEOir7Gu3pGpvfDUHbC5RUqk41QQmLm1ktV4 T6RIJRqX5+wE961dXQGvadrLhcw/dwF6QFH8XAJ8sG2S/mwSOgQfIjqS+PsJZInFlU2g qo8hBKM0RIn+xphBavqzOpKrKAep68hr4bR0b+Gw3Feb0ldHg3rbp+d4EtmEPsXtqeex dfkYPo0z3YYdfidVGRsaIwv1t17rLpNhLxCni+APS/AZIGiGuAqIbFocD03jSPSbuXkx IPCg== X-Gm-Message-State: ACrzQf06npSFUPrlDYed/xdhAHecU2YrnJGnfVu25VpVy9LZ2pGw32YU M1a5K4hZOoCHjUbfb+cK9/lIc55So9VdFAAgOR0LPTAuxaA2qOdCGEM4dL7yDgamvghp7YmvzV1 wd7o/EoPRutVRfnSp2PSkJlMgbWAAs9nJtgZAilAkQSsX97Sr0E5b0G5oeRa1STer X-Received: by 2002:a05:620a:2956:b0:6ce:60f5:d887 with SMTP id n22-20020a05620a295600b006ce60f5d887mr18126442qkp.303.1663714350079; Tue, 20 Sep 2022 15:52:30 -0700 (PDT) X-Google-Smtp-Source: AMsMyM49lITWSIq6Qf4eStlCqcoinipwVYURNvenIv6S2ySvesFAJ4U0gTAtHMOq9uUg1mM4c/vx6Q== X-Received: by 2002:a05:620a:2956:b0:6ce:60f5:d887 with SMTP id n22-20020a05620a295600b006ce60f5d887mr18126421qkp.303.1663714349600; Tue, 20 Sep 2022 15:52:29 -0700 (PDT) Received: from localhost.localdomain (bras-base-aurron9127w-grc-46-70-31-27-79.dsl.bell.ca. [70.31.27.79]) by smtp.gmail.com with ESMTPSA id z12-20020ac87f8c000000b0035ba7012724sm596479qtj.70.2022.09.20.15.52.28 (version=TLS1_3 cipher=TLS_CHACHA20_POLY1305_SHA256 bits=256/256); Tue, 20 Sep 2022 15:52:29 -0700 (PDT) From: Peter Xu To: qemu-devel@nongnu.org Cc: peterx@redhat.com, Manish Mishra , Juan Quintela , ani@anisinha.ca, Leonardo Bras Soares Passos , "Dr . David Alan Gilbert" , "Daniel P . Berrange" Subject: [PATCH 13/14] migration: Remove old preempt code around state maintainance Date: Tue, 20 Sep 2022 18:52:27 -0400 Message-Id: <20220920225227.49158-1-peterx@redhat.com> X-Mailer: git-send-email 2.32.0 In-Reply-To: <20220920225106.48451-1-peterx@redhat.com> References: <20220920225106.48451-1-peterx@redhat.com> MIME-Version: 1.0 Content-type: text/plain Received-SPF: pass client-ip=170.10.129.124; envelope-from=peterx@redhat.com; helo=us-smtp-delivery-124.mimecast.com X-Spam_score_int: -27 X-Spam_score: -2.8 X-Spam_bar: -- X-Spam_report: (-2.8 / 5.0 requ) BAYES_00=-1.9, DKIMWL_WL_HIGH=-0.001, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, DKIM_VALID_EF=-0.1, RCVD_IN_DNSWL_LOW=-0.7, SPF_HELO_NONE=0.001, SPF_PASS=-0.001 autolearn=ham autolearn_force=no X-Spam_action: no action X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org Sender: "Qemu-devel" With the new code to send pages in rp-return thread, there's little help to keep lots of the old code on maintaining the preempt state in migration thread, because the new way should always be faster.. Then if we'll always send pages in the rp-return thread anyway, we don't need those logic to maintain preempt state anymore because now we serialize things using the mutex directly instead of using those fields. It's very unfortunate to have those code for a short period, but that's still one intermediate step that we noticed the next bottleneck on the migration thread. Now what we can do best is to drop unnecessary code as long as the new code is stable to reduce the burden. It's actually a good thing because the new "sending page in rp-return thread" model is (IMHO) even cleaner and with better performance. Remove the old code that was responsible for maintaining preempt states, at the meantime also remove x-postcopy-preempt-break-huge parameter because with concurrent sender threads we don't really need to break-huge anymore. Signed-off-by: Peter Xu Reviewed-by: Dr. David Alan Gilbert --- migration/migration.c | 2 - migration/ram.c | 258 +----------------------------------------- 2 files changed, 3 insertions(+), 257 deletions(-) diff --git a/migration/migration.c b/migration/migration.c index fae8fd378b..698fd94591 100644 --- a/migration/migration.c +++ b/migration/migration.c @@ -4399,8 +4399,6 @@ static Property migration_properties[] = { DEFINE_PROP_SIZE("announce-step", MigrationState, parameters.announce_step, DEFAULT_MIGRATE_ANNOUNCE_STEP), - DEFINE_PROP_BOOL("x-postcopy-preempt-break-huge", MigrationState, - postcopy_preempt_break_huge, true), DEFINE_PROP_STRING("tls-creds", MigrationState, parameters.tls_creds), DEFINE_PROP_STRING("tls-hostname", MigrationState, parameters.tls_hostname), DEFINE_PROP_STRING("tls-authz", MigrationState, parameters.tls_authz), diff --git a/migration/ram.c b/migration/ram.c index fd301d793c..f42efe02fc 100644 --- a/migration/ram.c +++ b/migration/ram.c @@ -343,20 +343,6 @@ struct RAMSrcPageRequest { QSIMPLEQ_ENTRY(RAMSrcPageRequest) next_req; }; -typedef struct { - /* - * Cached ramblock/offset values if preempted. They're only meaningful if - * preempted==true below. - */ - RAMBlock *ram_block; - unsigned long ram_page; - /* - * Whether a postcopy preemption just happened. Will be reset after - * precopy recovered to background migration. - */ - bool preempted; -} PostcopyPreemptState; - /* State of RAM for migration */ struct RAMState { /* QEMUFile used for this migration */ @@ -419,14 +405,6 @@ struct RAMState { /* Queue of outstanding page requests from the destination */ QemuMutex src_page_req_mutex; QSIMPLEQ_HEAD(, RAMSrcPageRequest) src_page_requests; - - /* Postcopy preemption informations */ - PostcopyPreemptState postcopy_preempt_state; - /* - * Current channel we're using on src VM. Only valid if postcopy-preempt - * is enabled. - */ - unsigned int postcopy_channel; }; typedef struct RAMState RAMState; @@ -434,11 +412,6 @@ static RAMState *ram_state; static NotifierWithReturnList precopy_notifier_list; -static void postcopy_preempt_reset(RAMState *rs) -{ - memset(&rs->postcopy_preempt_state, 0, sizeof(PostcopyPreemptState)); -} - /* Whether postcopy has queued requests? */ static bool postcopy_has_request(RAMState *rs) { @@ -544,9 +517,6 @@ static int ram_save_host_page_urgent(PageSearchStatus *pss); static bool do_compress_ram_page(QEMUFile *f, z_stream *stream, RAMBlock *block, ram_addr_t offset, uint8_t *source_buf); -static void postcopy_preempt_restore(RAMState *rs, PageSearchStatus *pss, - bool postcopy_requested); - /* NOTE: page is the PFN not real ram_addr_t. */ static void pss_init(PageSearchStatus *pss, RAMBlock *rb, ram_addr_t page) { @@ -2062,55 +2032,6 @@ void ram_write_tracking_stop(void) } #endif /* defined(__linux__) */ -/* - * Check whether two addr/offset of the ramblock falls onto the same host huge - * page. Returns true if so, false otherwise. - */ -static bool offset_on_same_huge_page(RAMBlock *rb, uint64_t addr1, - uint64_t addr2) -{ - size_t page_size = qemu_ram_pagesize(rb); - - addr1 = ROUND_DOWN(addr1, page_size); - addr2 = ROUND_DOWN(addr2, page_size); - - return addr1 == addr2; -} - -/* - * Whether a previous preempted precopy huge page contains current requested - * page? Returns true if so, false otherwise. - * - * This should really happen very rarely, because it means when we were sending - * during background migration for postcopy we're sending exactly the page that - * some vcpu got faulted on on dest node. When it happens, we probably don't - * need to do much but drop the request, because we know right after we restore - * the precopy stream it'll be serviced. It'll slightly affect the order of - * postcopy requests to be serviced (e.g. it'll be the same as we move current - * request to the end of the queue) but it shouldn't be a big deal. The most - * imporant thing is we can _never_ try to send a partial-sent huge page on the - * POSTCOPY channel again, otherwise that huge page will got "split brain" on - * two channels (PRECOPY, POSTCOPY). - */ -static bool postcopy_preempted_contains(RAMState *rs, RAMBlock *block, - ram_addr_t offset) -{ - PostcopyPreemptState *state = &rs->postcopy_preempt_state; - - /* No preemption at all? */ - if (!state->preempted) { - return false; - } - - /* Not even the same ramblock? */ - if (state->ram_block != block) { - return false; - } - - return offset_on_same_huge_page(block, offset, - state->ram_page << TARGET_PAGE_BITS); -} - /** * get_queued_page: unqueue a page from the postcopy requests * @@ -2150,20 +2071,7 @@ static bool get_queued_page(RAMState *rs, PageSearchStatus *pss) } while (block && !dirty); - if (block) { - /* See comment above postcopy_preempted_contains() */ - if (postcopy_preempted_contains(rs, block, offset)) { - trace_postcopy_preempt_hit(block->idstr, offset); - /* - * If what we preempted previously was exactly what we're - * requesting right now, restore the preempted precopy - * immediately, boosting its priority as it's requested by - * postcopy. - */ - postcopy_preempt_restore(rs, pss, true); - return true; - } - } else { + if (!block) { /* * Poll write faults too if background snapshot is enabled; that's * when we have vcpus got blocked by the write protected pages. @@ -2433,129 +2341,6 @@ static int ram_save_target_page(RAMState *rs, PageSearchStatus *pss) return ram_save_page(rs, pss); } -static bool postcopy_needs_preempt(RAMState *rs, PageSearchStatus *pss) -{ - MigrationState *ms = migrate_get_current(); - - /* Not enabled eager preempt? Then never do that. */ - if (!migrate_postcopy_preempt()) { - return false; - } - - /* If the user explicitly disabled breaking of huge page, skip */ - if (!ms->postcopy_preempt_break_huge) { - return false; - } - - /* If the ramblock we're sending is a small page? Never bother. */ - if (qemu_ram_pagesize(pss->block) == TARGET_PAGE_SIZE) { - return false; - } - - /* Not in postcopy at all? */ - if (!migration_in_postcopy()) { - return false; - } - - /* - * If we're already handling a postcopy request, don't preempt as this page - * has got the same high priority. - */ - if (pss->postcopy_requested) { - return false; - } - - /* If there's postcopy requests, then check it up! */ - return postcopy_has_request(rs); -} - -/* Returns true if we preempted precopy, false otherwise */ -static void postcopy_do_preempt(RAMState *rs, PageSearchStatus *pss) -{ - PostcopyPreemptState *p_state = &rs->postcopy_preempt_state; - - trace_postcopy_preempt_triggered(pss->block->idstr, pss->page); - - /* - * Time to preempt precopy. Cache current PSS into preempt state, so that - * after handling the postcopy pages we can recover to it. We need to do - * so because the dest VM will have partial of the precopy huge page kept - * over in its tmp huge page caches; better move on with it when we can. - */ - p_state->ram_block = pss->block; - p_state->ram_page = pss->page; - p_state->preempted = true; -} - -/* Whether we're preempted by a postcopy request during sending a huge page */ -static bool postcopy_preempt_triggered(RAMState *rs) -{ - return rs->postcopy_preempt_state.preempted; -} - -static void postcopy_preempt_restore(RAMState *rs, PageSearchStatus *pss, - bool postcopy_requested) -{ - PostcopyPreemptState *state = &rs->postcopy_preempt_state; - - assert(state->preempted); - - pss->block = state->ram_block; - pss->page = state->ram_page; - - /* Whether this is a postcopy request? */ - pss->postcopy_requested = postcopy_requested; - /* - * When restoring a preempted page, the old data resides in PRECOPY - * slow channel, even if postcopy_requested is set. So always use - * PRECOPY channel here. - */ - pss->postcopy_target_channel = RAM_CHANNEL_PRECOPY; - - trace_postcopy_preempt_restored(pss->block->idstr, pss->page); - - /* Reset preempt state, most importantly, set preempted==false */ - postcopy_preempt_reset(rs); -} - -static void postcopy_preempt_choose_channel(RAMState *rs, PageSearchStatus *pss) -{ - MigrationState *s = migrate_get_current(); - unsigned int channel = pss->postcopy_target_channel; - QEMUFile *next; - - if (channel != rs->postcopy_channel) { - if (channel == RAM_CHANNEL_PRECOPY) { - next = s->to_dst_file; - } else { - next = s->postcopy_qemufile_src; - } - /* Update and cache the current channel */ - rs->f = next; - rs->postcopy_channel = channel; - - /* - * If channel switched, reset last_sent_block since the old sent block - * may not be on the same channel. - */ - pss->last_sent_block = NULL; - - trace_postcopy_preempt_switch_channel(channel); - } - - trace_postcopy_preempt_send_host_page(pss->block->idstr, pss->page); -} - -/* We need to make sure rs->f always points to the default channel elsewhere */ -static void postcopy_preempt_reset_channel(RAMState *rs) -{ - if (postcopy_preempt_active()) { - rs->postcopy_channel = RAM_CHANNEL_PRECOPY; - rs->f = migrate_get_current()->to_dst_file; - trace_postcopy_preempt_reset_channel(); - } -} - /* Should be called before sending a host page */ static void pss_host_page_prepare(PageSearchStatus *pss) { @@ -2677,11 +2462,6 @@ static int ram_save_host_page(RAMState *rs, PageSearchStatus *pss) pss_host_page_prepare(pss); do { - if (postcopy_needs_preempt(rs, pss)) { - postcopy_do_preempt(rs, pss); - break; - } - page_dirty = migration_bitmap_clear_dirty(rs, pss->block, pss->page); /* * Properly yield the lock only in postcopy preempt mode because @@ -2723,19 +2503,6 @@ static int ram_save_host_page(RAMState *rs, PageSearchStatus *pss) pss_host_page_finish(pss); - /* - * When with postcopy preempt mode, flush the data as soon as possible for - * postcopy requests, because we've already sent a whole huge page, so the - * dst node should already have enough resource to atomically filling in - * the current missing page. - * - * More importantly, when using separate postcopy channel, we must do - * explicit flush or it won't flush until the buffer is full. - */ - if (migrate_postcopy_preempt() && pss->postcopy_requested) { - qemu_fflush(pss->pss_channel); - } - res = ram_save_release_protection(rs, pss, start_page); return (res < 0 ? res : pages); } @@ -2783,24 +2550,11 @@ static int ram_find_and_save_block(RAMState *rs) found = get_queued_page(rs, pss); if (!found) { - /* - * Recover previous precopy ramblock/offset if postcopy has - * preempted precopy. Otherwise find the next dirty bit. - */ - if (postcopy_preempt_triggered(rs)) { - postcopy_preempt_restore(rs, pss, false); - found = true; - } else { - /* priority queue empty, so just search for something dirty */ - found = find_dirty_block(rs, pss, &again); - } + /* priority queue empty, so just search for something dirty */ + found = find_dirty_block(rs, pss, &again); } if (found) { - /* Update rs->f with correct channel */ - if (postcopy_preempt_active()) { - postcopy_preempt_choose_channel(rs, pss); - } /* Cache rs->f in pss_channel (TODO: remove rs->f) */ pss->pss_channel = rs->f; pages = ram_save_host_page(rs, pss); @@ -2932,8 +2686,6 @@ static void ram_state_reset(RAMState *rs) rs->last_page = 0; rs->last_version = ram_list.version; rs->xbzrle_enabled = false; - postcopy_preempt_reset(rs); - rs->postcopy_channel = RAM_CHANNEL_PRECOPY; } #define MAX_WAIT 50 /* ms, half buffered_file limit */ @@ -3577,8 +3329,6 @@ static int ram_save_iterate(QEMUFile *f, void *opaque) } qemu_mutex_unlock(&rs->bitmap_mutex); - postcopy_preempt_reset_channel(rs); - /* * Must occur before EOS (or any QEMUFile operation) * because of RDMA protocol. @@ -3656,8 +3406,6 @@ static int ram_save_complete(QEMUFile *f, void *opaque) return ret; } - postcopy_preempt_reset_channel(rs); - ret = multifd_send_sync_main(rs->f); if (ret < 0) { return ret;