Attention is currently required from: fixeria.

View Change

4 comments:

• File libosmo-trx/include/osmocom/trx/ep.h:

  • Patch Set #1, Line 27: struct osmo_trx_ep_cfg {

    Let's please have separate setter functions for all these, like osmo_stream, so we don't need to break ABI in the future.

• File libosmo-trx/src/trx_ep.c:

  • Patch Set #1, Line 60: uint8_t pdu_ver; /* TRXD PDU version in use */

    iirc PDU VER stuff is defined/envisioned only for TRX0, so this can be moved to struct osmo_trx_ep.

  • Patch Set #1, Line 70: struct osmo_trx_ep_chan *chans; /* array of cfg.num_chans channels */

    If the chans array is allocated at the end of the struct, it probably makes sense to use "struct osmo_trx_ep_chan chans[]" here?

  • Patch Set #1, Line 302: ep->chans = talloc_zero_array(ep, struct osmo_trx_ep_chan, cfg->num_chans);

    Ah I see you are allocating the array here as a pointer to a seaprate memory region.
    It probably makes sense to allocate it at the end just by defining it with the array [0] (see comment at the start of the file), and then doing:

    ep = talloc_zero(ctx, sizeof(struct osmo_trx_ep) + sizeof(struct osmo_trx_ep_chan) * cfg->num_chans);

    This way you avoid fragmenting memory and probably already preload mem cache of the channels, which may be useful here for performance of trxd. Up to you.

To view, visit change 43111. To unsubscribe, or for help writing mail filters, visit settings.