Change in osmo-hnodeb[master]: First implementation of the LLSK gtp SAPI

[laforge]

laforge has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-hnodeb/+/26503 )

Change subject: First implementation of the LLSK gtp SAPI
......................................................................


Patch Set 2: Code-Review-1

(6 comments)

https://gerrit.osmocom.org/c/osmo-hnodeb/+/26503/2/include/osmocom/hnodeb/hnb_prim.h 
File include/osmocom/hnodeb/hnb_prim.h:

https://gerrit.osmocom.org/c/osmo-hnodeb/+/26503/2/include/osmocom/hnodeb/hnb_prim.h@243 
PS2, Line 243: uint8_t remote_gtpu_address_type;
             : 	union u_addr remote_gtpu_addr;
same comment about address like in the RTP patch


https://gerrit.osmocom.org/c/osmo-hnodeb/+/26503/2/include/osmocom/hnodeb/hnb_prim.h@286 
PS2, Line 286: 	un
same comment about moving context_id up one layer as in RTP patch


https://gerrit.osmocom.org/c/osmo-hnodeb/+/26503/2/src/osmo-hnodeb/gtp.c 
File src/osmo-hnodeb/gtp.c:

https://gerrit.osmocom.org/c/osmo-hnodeb/+/26503/2/src/osmo-hnodeb/gtp.c@63 
PS2, Line 63: 	LOG
not sure we want that level of verbosity in a user plane code path? you will get a Tx... line at the same log level with the same information 7 lines below.


https://gerrit.osmocom.org/c/osmo-hnodeb/+/26503/2/src/osmo-hnodeb/gtp.c@70 
PS2, Line 70: req
comment says request, next line says indication.  Further comment below again says req.

One ideq aould be to move the logging of the message type into a log macro, which simply uses a value_string to print the message type?  Not runtime efficient due to the value_string lookup, but neither DEBUG nor ERROR should happen _that_ often in a production deployment?


https://gerrit.osmocom.org/c/osmo-hnodeb/+/26503/2/src/osmo-hnodeb/gtp.c@126 
PS2, Line 126: }
             : 
             : 	osmo_fd_setup(&hnb->gtp.fd0, gsn->fd0, OSMO_FD_READ, hnb_gtp_fd_cb, hnb, 0);
             : 	if ((rc = osmo_fd_register(&hnb->gtp.fd0)) < 0)
             : 		goto free_ret;
             : 
             : 	osmo_fd_setup(&hnb->gtp.fd1c, gsn->fd1c, OSMO_FD_READ, hnb_gtp_fd_cb, hnb, 1);
             : 	if ((rc = osmo_fd_register(&hnb->gtp.fd1c)) < 0)
             : 		goto free_ret;
             : 
             : 	osmo_fd_setup(&hnb->gtp.fd1u, gsn->fd1u, OSMO_FD_READ, hnb_gtp_fd_cb, hnb, 2);
             : 	if ((rc = osmo_fd_register(&hnb->gtp.fd1u)) < 0)
The Iuh interface only uses GTP-U v1, neither GTPv0 and no GTPv1-C.  So why are we creating three sockets and instantianting an entire GSN here?

The HNB is no GSN.  GSNs are SGSN and GGSN.

I don't see the advantage of using libgtp here.  What is the rationale of using it?  AFAICT all we need for GTPv1U is to push one static, fixed size GTP header in front of every TX packet, and parse/remove the same fixed-size header in the RX direction.

I'm afraid by uising a complete "GSN" implementation we get a lot of stuff that we don't want here, and which might create trouble.  But maybe you did a thorough study of libgtp and decided it is a better option?


https://gerrit.osmocom.org/c/osmo-hnodeb/+/26503/2/src/osmo-hnodeb/gtp.c@180 
PS2, Line 180: 		LOG
where deos this constraint come from? libgtp?



-- 
To view, visit https://gerrit.osmocom.org/c/osmo-hnodeb/+/26503
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings

Gerrit-Project: osmo-hnodeb
Gerrit-Branch: master
Gerrit-Change-Id: I5a6f5dfc4e508c92adb35210b4dc576d64353366
Gerrit-Change-Number: 26503
Gerrit-PatchSet: 2
Gerrit-Owner: pespin <pespin at sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: laforge <laforge at osmocom.org>
Gerrit-Comment-Date: Sun, 12 Dec 2021 08:00:24 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osmocom.org/pipermail/gerrit-log/attachments/20211212/42011ee0/attachment.htm>