On Mon, Nov 24, 2025 at 07:01:14PM +0100, Oleg Nesterov wrote:
> Hi Jiri,
>
> I am trying to understand this series, will try to read it more carefully
> later...
>
> (damn why do you always send the patches when I am on PTO? ;)
it's more fun that way ;-) thanks for checking on it
>
> On 11/17, Jiri Olsa wrote:
> >
> > struct arch_uprobe {
> > union {
> > - u8 insn[MAX_UINSN_BYTES];
> > + u8 insn[5*MAX_UINSN_BYTES];
>
> Hmm. OK, this matches the "for (i = 0; i < 5; i++)" loop in
> opt_setup_xol_ops(), but do we really need this change? Please see
> the question at the end.
>
> > +static int opt_setup_xol_ops(struct arch_uprobe *auprobe, struct insn
> > *insn)
> > +{
> > + unsigned long offset = insn->length;
> > + struct insn insnX;
> > + int i, ret;
> > +
> > + if (test_bit(ARCH_UPROBE_FLAG_CAN_OPTIMIZE, &auprobe->flags))
> > + return -ENOSYS;
>
> I think this logic needs some cleanups... If ARCH_UPROBE_FLAG_CAN_OPTIMIZE
> is set by the caller, the it doesn't make sense to call xxx_setup_xol_ops(),
> right? But lets forget it for now.
>
> > + ret = opt_setup_xol_insns(auprobe, &auprobe->opt.xol[0], insn);
>
> I think this should go into the main loop, see below
>
> > + for (i = 1; i < 5; i++) {
> > + ret = uprobe_init_insn_offset(auprobe, offset, &insnX, true);
> > + if (ret)
> > + break;
> > + ret = opt_setup_xol_insns(auprobe, &auprobe->opt.xol[i],
> > &insnX);
> > + if (ret)
> > + break;
> > + offset += insnX.length;
> > + auprobe->opt.cnt++;
> > + if (offset >= 5)
> > + goto optimize;
> > + }
> > +
> > + return -ENOSYS;
>
> I don't think -ENOSYS makes sense if opt_setup_xol_insns() succeeds at least
> once.
> IOW, how about
>
> static int opt_setup_xol_ops(struct arch_uprobe *auprobe, struct insn
> *insn)
> {
> unsigned long offset = 0;
> struct insn insnX;
> int i, ret;
>
> if (test_bit(ARCH_UPROBE_FLAG_CAN_OPTIMIZE, &auprobe->flags))
> return -ENOSYS;
>
> for (i = 0; i < 5; i++) {
> ret = opt_setup_xol_insns(auprobe,
> &auprobe->opt.xol[i], insn);
> if (ret)
> break;
> offset += insn->length;
> if (offset >= 5)
> break;
>
> insn = &insnX;
> ret = uprobe_init_insn_offset(auprobe, offset, insn,
> true);
> if (ret)
> break;
> }
>
> if (!offset)
> return -ENOSYS;
>
> if (offset >= 5) {
> auprobe->opt.cnt = i + 1;
> auprobe->xol.ops = &opt_xol_ops;
> set_bit(ARCH_UPROBE_FLAG_CAN_OPTIMIZE, &auprobe->flags);
> set_bit(ARCH_UPROBE_FLAG_OPTIMIZE_EMULATE,
> &auprobe->flags);
> }
>
> return 0;
> }
>
> ?
>
> This way the caller, arch_uprobe_analyze_insn(), doesn't need to call
> push/mov/sub/_setup_xol_ops(), and the code looks a bit simpler to me.
ah nice, will try that
>
> No?
>
> > + * TODO perhaps we could 'emulate' nop, so there would be no need for
> > + * ARCH_UPROBE_FLAG_OPTIMIZE_EMULATE flag, because we would emulate
> > + * allways.
>
> Agreed... and this connects to "this logic needs some cleanups" above.
> I guess we need nop_setup_xol_ops() extracted from branch_setup_xol_ops()
> but again, lets forget it for now.
ok, it will hopefully make the code simpler, will check on that
>
> -------------------------------------------------------------------------------
> Now the main question. What if we avoid this change
>
> - u8 insn[MAX_UINSN_BYTES];
> + u8 insn[5*MAX_UINSN_BYTES];
>
> mentioned above, and change opt_setup_xol_ops() to just do
>
> - for (i = 0; i < 5; i++)
> + for (i = 0;; i++)
>
> ?
>
> The main loop stops when offset >= 5 anyway.
>
> And. if auprobe->insn[offset:MAX_UINSN_BYTES] doesn't contain a full/valid
> insn at the start, then uprobe_init_insn_offset()->insn_decode() should fail?
>
> Most probably I missed something, but I can't understand this part.
no, I think you're right, I did not realize we fit under MAX_UINSN_BYTES
anyway, call instruction needs only 5 bytes
thanks,
jirka
