Re: [PATCH v2] QIOChannelSocket: Flush zerocopy socket error queue on ENOBUF failure for sendmsg

[Peter Xu <peterx@redhat.com>]

On Tue, Mar 11, 2025 at 08:08:02PM +0000, Daniel P. Berrangé wrote:
> On Tue, Mar 11, 2025 at 03:57:35PM -0400, Peter Xu wrote:
> > On Tue, Mar 11, 2025 at 03:33:23PM +0000, Daniel P. Berrangé wrote:
> > > On Tue, Mar 11, 2025 at 11:20:50AM -0400, Peter Xu wrote:
> > > > On Tue, Mar 11, 2025 at 08:13:16AM +0000, Daniel P. Berrangé wrote:
> > > > > On Mon, Mar 10, 2025 at 04:03:26PM -0400, Peter Xu wrote:
> > > > > > On Mon, Mar 10, 2025 at 07:48:16PM +0000, Daniel P. Berrangé wrote:
> > > > > > > Given this is in public API, the data needs to remain reported accurately
> > > > > > > for the whole deprecation period. IOW, the patch to qiochannel needs to
> > > > > > > preserve this data too.
> > > > > > 
> > > > > > :-(
> > > > > > 
> > > > > > We could potentially mark MigrationStats to be experimental as a whole and
> > > > > > declare that in deprecate.rst too, then after two releases, we can randomly
> > > > > > add / remove fields as wish without always need to go through the
> > > > > > deprecation process, am I right?
> > > > > 
> > > > > IMHO that would be an abuse of the process and harmful to applications
> > > > > and users consuming stats.
> > > > 
> > > > Ah I just noticed that's the exact same one we included in
> > > > query-migrate.. Then yes, the stable ABI is important here.
> > > > 
> > > > So for this specific case, maybe we shouldn't have exposed it in QMP from
> > > > the start.
> > > > 
> > > > To me, it's a question on whether we could have something experimental and
> > > > be exposed to QMP, where we don't need to guarantee a strict stable ABI, or
> > > > a very loose ABI (e.g. we can guarantee the command exists, and with
> > > > key-value string-integer pairs, nothing else).
> > > 
> > > QMP has the ability to tag commands/fields, etc as experimental.
> > > 
> > > libvirt will explicitly avoid consuming or exposing anything with
> > > an experimental tag on it.
> > > 
> > > > Maybe what we need is a new MigrationInfoOptional, to be embeded into
> > > > MigrationInfo (or not), marked experimental.  Then in the future whenever
> > > > we want to add some new statistics, we could decide whether it should be
> > > > part of stable ABI or not.
> > > 
> > > That is not required - individual struct fields can be marked
> > > experimental.
> > 
> > Yes that'll work too.  The important bit here is I think we should start to
> > seriously evaluate which to expose to QAPI as stable API when we add stats
> > into it.  We used to not pay too much attention.
> > 
> > With MigrationInfoOptional, we should suggest any new field to be added
> > there by default, then whatever needs to be put out of experimental needs
> > explicit justifications.  Or we can also document any new migration field
> > at least in the stats to be marked as experimental unless justified.
> > 
> > > 
> > > The key question is what the intended usage of the fields/stats/etc
> > > is to be. If you want it used by libvirt and mgmt apps it would need
> > > to be formally supported. If it is just for adhoc QEMU developer
> > > debugging and doesn't need libvirt / app support, then experimental
> > > is fine.
> > 
> > To my initial thoughts, I want Libvirt to fetch it.  However I don't want
> > Libvirt to parse it.
> > 
> > For example, for things like "whether zerocopy send succeeded or not", or
> > "how much time we spent on sending non-iterable device states", they're
> > almost not consumable for users, but great for debuggings.  It would be
> > great if Libvirt could know their existance, fetch it (e.g. once after
> > migration completes) then dump it to the logfile to help debugging and
> > triaging QEMU issues.  In that case parsing is not needed, the whole result
> > can be attached to the log as a JSON blob.  That releases the burden from
> > the need to maintain compatibility that we don't really need and nobody
> > cared (I bet it's the case here for zerocopy stats, but we got restricted
> > by our promises even if it may ultimately benefit nobody..).
> 
> We already log every single QMP command & response and event we deal
> with, at INFO level, but by default our log files are only set to
> capture WARN level, so this isn't visible without extra config steps
> by the ademin
> 
> Possibly we could think about dumping all migration stats to
> /var/log/libvirt/qemu/$GUEST.log at migration completion

Yes it would be great to have it if it's trivial to get.  It could be a
last round of 'query-migrate' dumped only on src after migration is
completed, right before src QEMU shuts down.

Thanks,

-- 
Peter Xu